Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a way to prevent pakettic from reordering certain expressions (was: Lua issue with table access expression reordering) #15

Open
AMcBain opened this issue Jan 14, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@AMcBain
Copy link

AMcBain commented Jan 14, 2024

I have some code that, at least in my original TIC-80 program works perfectly but after minification (with default settings) it fails with invalid order function for sorting.

tl;dr: The problem isn't the minification per se, it's that certain table accesses if not done in one of a few specific orders results in different behavior/output.


Original code (works):

table.sort(b0,function (a,b)
  ax=(a[1].x+a[2].x+a[3].x)/3
  ay=(a[1].y+a[2].y+a[3].y)/3
  az=(a[1].z+a[2].z+a[3].z)/3
  bx=(b[1].x+b[2].x+b[3].x)/3  
  by=(b[1].y+b[2].y+b[3].y)/3
  bz=(b[1].z+b[2].z+b[3].z)/3
  if az~=bz then
   return az>bz
  end
  if ax~=bx then
   return ax<bx
  end
  return ay>by
 end)

b0 is a table that contains triangles ({{x=1.0,y=1.0,z=1.0}}) that have doubles as property values.

Minified code (fails):

table.sort(b0,function(a,b)
 ax=(a[3].x+a[2].x+a[1].x)/3
 ay=(a[3].y+a[2].y+a[1].y)/3
 az=(a[3].z+a[2].z+a[1].z)/3
 bx=(b[2].x+b[3].x+b[1].x)/3
 by=(b[2].y+b[3].y+b[1].y)/3
 bz=(b[2].z+b[1].z+b[3].z)/3
 if az~=bz then return bz<az end
 if bx~=ax then return ax<bx end
 return by<ay end
)

This has been reformatted for clarity and the variables renamed to match original, but I've tested to ensure the error still occurs. I also verified the comparison "flip" is not the cause either since it maintains the original meaning.


The Fix

 bz=(b[2].z+b[3].z+b[1].z)/3

This reorders the table access for line 6 of the sort function to match the two above it. Then it doesn't fail.

Buuuut I still don't have any d-mn idea why that fixes it because if go grab my original code and pair it with the minified code:

 ax=(a[3].x+a[2].x+a[1].x)/3
 ay=(a[3].y+a[2].y+a[1].y)/3
 az=(a[3].z+a[2].z+a[1].z)/3
 bx=(b[1].x+b[2].x+b[3].x)/3
 by=(b[1].y+b[2].y+b[3].y)/3
 bz=(b[1].z+b[2].z+b[3].z)/3

It now fails!

But this works!

 bx=(b[3].x+b[2].x+b[1].x)/3
 by=(b[3].y+b[2].y+b[1].y)/3
 bz=(b[2].z+b[3].z+b[1].z)/3

I am so confused.

This issue isn't really the minifiers fault, since I can trigger it in unminified code, but there's two-three issues that may be:

  1. The minifier doesn't seem to care if I put --{! and --} around entire table.sort block, and reorders stuff inside anyway.
  2. Even if I reorder the non-minified accesses to match the order of the initial minified output, minifying will still reorder them.
  3. I had a working minified version before moving a couple unrelated statements elsewhere in the file, so it only messes with these arbitrarily.

-- Starchaser

@AMcBain
Copy link
Author

AMcBain commented Jan 14, 2024

As an update, if it helps any, pakettic file.lua.tic -aanneal does appear to generate the smallest output and leaves the ordering of those alone (or at least picks an ordering that doesn't make Lua complain.

@vsariola
Copy link
Owner

-aanneal is probably just lucky and does not produce invalid code. I agree, this is super weird. I'll investigate. Meanwhile, can you send the whole offending code? This might have something to do with the specific tables you have. You can DM me on discord if you want more real-time debugging session.

Oh, are you on 1.3.1?

@vsariola
Copy link
Owner

vsariola commented Jan 15, 2024

Regarding --{!: it does not prevent expression reordering. It only prevents reordering statements, which are not normally reordered, but are only reordered within --{ block. So, --{! has no use unless used within a --{ block. There is no way preventing expression reordering; maybe there should be because one day a case will arise where reordering expressions breaks the code, but I am not convinced yet that yours is one: it makes no sense for that reordering to break the code.

@AMcBain
Copy link
Author

AMcBain commented Jan 15, 2024

-aanneal is probably just lucky and does not produce invalid code. I agree, this is super weird. I'll investigate. Meanwhile, can you send the whole offending code? This might have something to do with the specific tables you have. You can DM me on discord if you want more real-time debugging session.

Oh, are you on 1.3.1?

I look to be, based on my env's site packages. I only installed it a couple days ago. As to aanneal, I think so too since I also got it to bug once. Just lucky it decided that wasn't a better optimization.

Regarding --{!: it does not prevent expression reordering. It only prevents reordering statements, which are not normally reordered, but are only reordered within --{ block. So, --{! has not use unless used within a --{ block. There is no way preventing expression reordering; maybe there should be because I one day a case will arise where reordering expressions breaks the code, but I am not convinced yet that yours is one: it makes no sense for that reordering to break the code.

I did actually have a --{ block surrounding the code I tried putting --{! around, as I grouped two large sets of code separately. (Though later determined I don't need to, but strangely things packs better if I keep that arrangement).


I did later determine the the /3 isn't necessary and can be removed while still being bugged. Someone did point out this might just be a a "weird" floating point issue, though I would hope that simple addition is stable, such that a + b + c is the same as b + a + c every time it's run. Because while floating point can be a PITA (see 0.2 + 0.1), the math should always be the same.

I was able to work around this problem by a suggestion of just looping through and precaulating the added values before the sort so it's only ever run once per sort.

I'll sort out sending you a copy since I still hold out a little hope I can figure out how to get my code ≤ 1kb for something upcoming. 🙂

@vsariola
Copy link
Owner

vsariola commented Jan 15, 2024

a+b+c != a+c+b in general. For example: a and b can be both so small that when added to a big number c, it rounds back to c. But when a & b are first added together, it becomes just big enough number to notch c up by tiny amount. Therefore, if ax, ay, az are calculated using one set of equations and bx, by and bz calculated using another set of equations, the sorting result might depend on which element is a and which element is b. And thus, LUA correctly complains that the sorting function is invalid, because it is :) The sorting function should not depend which element is which.

One ugly idea: make one function to return ax, ay, az that is called for a and b separately. This guarantees that the values are calculated identically.

True sizecoding way: just sort by the zs of the first vertices and be done with it :D

@vsariola vsariola added the enhancement New feature or request label Jan 15, 2024
@vsariola
Copy link
Owner

Well, this is the first example where pakettic reordering broke the code. Granted, we were able to find ways around it, but I'll change this into a feature request: add a way to prevent pakettic from reordering certain expressions. More magic comments are needed; I'll spend some time to think what's actually the good notation for it.

@vsariola vsariola changed the title Lua issue with table access expression reordering Add a way to prevent pakettic from reordering certain expressions (was: Lua issue with table access expression reordering) Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants