search

astuder / lightroom-map-fix

Fixing the Map module in Lightroom Classic
125 stars 17 forks source link

Read system parameters order was incorrect. #6

Closed dhoepfl closed 5 years ago

dhoepfl commented 5 years ago

The comment (correctly) states that the system parameter order is (endianess, int size, instruction size, size_t size, lua num size, float) but the code read (endian, int size, size_t size, instruction size, instruction size).

(Change is untested)

astuder commented 5 years ago

Thank you for your pull-request.

I agree with your change for reading the 4th parameter (lua_num_size). This is a bug.

However, I found conflicting information about size_t and instruction size. Some say that the order is int, size_t, instruction, number, others say it's int, instruction, size_t, number. Given that in the Lua files at hand, the strings use 8-byte integers to store string length, I did go with the first.

Just checked the source code of Lua 5.1, and it's int, size_t, instruction, number.

I also tested your patch with a Lua file from Lightroom: Instruction size is 8 bytes, size_t is 4 bytes, parsing of Lua strings fails.

So, I guess the comment is wrong but the code was correct, except for the bug with lua_num_size.

astuder commented 5 years ago

Let me know if you want to update your pull request, or if I should go ahead and close your pull request and make the changes directly in my branch.

dhoepfl commented 5 years ago

Just close the pull request.

Fun-Fact: While I could not test my fix since I did not have Lightroom at hands to do so, I double-checked the order against two or three "Lua Object File Spec" hits …

(Plus: HUGE Thank you for this work.)