-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Document integer data types and ranges in Lua API #16735
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for documenting this! I will try to take a comprehensive look soon. Some thoughts: For all practical purposes, we can assume IEEE-754. But the more limiting factor is usually going to be a cast to some 32-bit (or even smaller) integer type on the C++ side. This is what I wrote on the matter in the other repo: https://docs.luanti.org/for-creators/lua-conventions/#integers
Then let's make it use |
|
It has been brought to my attention that Luanti essentially "forces" Good point about IEEE 754. Yeah, I'm probably being overly paranoid here. I can't recall any relevant system where a Please note that I sometimes took the liberty to document only a 16-bit range when theoretically a 32-bit range is possible. This was the case where my tests indicate Luanti behaves poorly with large values (i.e. very poor performance, lag spikes), or a value beyond 16-bit does not make even theoretical sense in that case. Please complain about all 16-bit ranges in the documentation where you think I was too restrictive. But more importantly, we must be extra sure that no range in the documentation is larger than is actually supported because that will be a BugFactory. |
appgurueu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks solid, some small things i found while reading through it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought: Could you make explicit that VoxelAreas expect integer positions (in bounds)? It seems to be a frequent mistake that people pass non-integer positions and end up surprised by nodes at wrong places.
|
OK, I addressed all comments. I have added shorthands for common integer ranges like I am not touching the Good catch about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job and thanks for taking on this! here are some typos and my attempts at making this sound better (though keep in mind that I am too only a second-language English speaker).
BTW, you could mention that some Lua 5.1 APIs are ambiguous on whether they take any real number or an integer and that behavior may vary there too from implementation to implementation (and to look out for that), e.g. with when floats are passed to math.random in PUC Lua 5.1 and LuaJIT 2.1. maybe (and I'm totally not doing this to substitute cat picture payments to @appgurueu) you could mention that there are tools like Runtime Strictness to catch undefined behaviors in both Lua and Luanti API? it would be a good idea to refer scripters to RTFM, but when the whole ordeal is that a part of the API is undocumented, there's really no FM to R specifically...
|
All direct code comments by @wmikita addressed. Thank you for looking at the small details, it is important to be precise.
Not sure, this might be overkill for a reference. I don't think we have to also document every Lua quirk as well.
Not sure about that, I don't know how useful this tool is. Maybe a good recommentation in docs.luanti.org tho (if it's a good tool). Question to everyone important in Luanti: Is it safe to assume that in Luanti, |
j-r
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments concerning the world seed (also named map seed in at least one place) and block positions.
|
New comments addressed. I also decided to rename all ranges to the more familiar Luanti C++ style Question to everyone important in Luanti: Is it safe to assume that in Luanti, |
|
Thank you for working on this, this will definitely prevent some breakage. |
|
Added the missing documentation for texture modifier sizes and coordinates. New Still waiting for an answer for what the "official" safe integer range should be (see above). |
|
appgurueu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looked at the changes since my last review, looks good, i have some minor comments
|
@appgurueu comments addressed. |
corpserot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i managed to review all the way to the end of Flag Specifier Format section today. seems like this PR won't be easy to be thorough. i hope floats/lua numbers are considered in-scope of this PR considering your rationale.
doc/lua_api.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the expected number ranges for selection boxes? is it different between nodes and entities? can you have it documented? from quick testing without fiddling with any server/client settings and without forceloads, i got a selection box that works fine up to 96 nodes tall from an entity's origin. it's something i've never been able to figure out.
also collision boxes should describe its range with the PR's range format instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this one is tricky. I agree specifying a range for selection boxes is important but I have no idea what it is. Clearly it cannot be infinite.
I believe I saw a #define in the code somewhere but maybe I misremember things.
I also believe that entities allow much larger collision/selection boxes than nodes to, but I don't know the limits for them as well. The only thing that is certain is that node boxes use floating point coordinates.
Or maybe the limits never have been determined to begin with which would mean we would have to decide on limits from scratch.
Unless someone outright tells me the correct range, this is out of scope. This sounds like something for a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same here) that's alright. i suppose i was hoping for more considering the motivations you had.
|
@corpserot comments addressed. I agree that float ranges should also be specified but sadly they are much harder to figure out. Specifying ranges that have not been specified anywhere before is out of scope for this PR. E.g. I would love to know the correct Here's the deal: |
|
@Wuzzy2 rebase needed |
c03b608 to
a16cbd5
Compare
|
Rebase complete. I removed the There is still an open question from the 1st post:
Relevant code in |
This a large documentation PR about integers. It took me almost a whole day to write.
It updates the
lua_api.mddocumentation significantly by adding a distinction between "number" and "integer" in the documentation, and documenting value ranges.Rationale
Many Luanti functions and data structures expect integers and the C++ code can only accept a limited value range. The Lua API documentation rarely mentions these restrictions which has led to some confusion in the past.
Worse, Lua programmers passing floating-point or out-of-range values to functions or data structures that expect integers can potentially lead to bugs without the programmer realizing, because it was never documented.
Real example: In one game, I have found a
stack_maxof 99999, which is not a supported value but the programmer can't be blamed because the upper bound was not specified there.The goal of this PR is to make Lua code more robust longterm by mentioning our number restrictions upfront.
Overview
This PR is structured in multiple parts:
numberas C++doubleis mentioned here (seeLUA_NUMBERinlib/lua/src/luaconf.h)[min, max][-2^53, 2^53]in the Numbers and Integers section for the few cases where no range was mentioned. This safe integer range is the range that Luanti will now officially guarantee that all integers in this range can be represented in Lua numbers without "holes". This information is relevant for builtin functionsThe information about the integer types and value ranges is derived from the C++ code.
Noteworthy details for review
To the reviewers I want to bring special attention to these points:
I picked a "pessimistic" range of(raised to[-2^37, 2^37]for the safe integer range. This one is derived from the lowest guaranteed value ofDBL_MANT_DIGin<float.c>.[-2^53, 2^53])I mentioned that if IEEE-754 floating-point numbers are supported, the safe integer range is(no longer relevant)[-2^53, 2^53]. Why 53? Because 53 is the 52-bit mantissa of double-precision IEEE-754 floats plus 1 extra bit because the specification implies there is an extra1bit at the beginning of the mantissa that is not actually stored.(I don't know if Luanti ALWAYS has IEEE-754 floats (thus making this distinction moot) or if it is possible to compile Luanti with lower float ranges)(the answer is, for all practical intents and purposes, yes)set_intof metadata storage, I made the value range explicitly 32-bit, rather than this just being a suggestioncore.set_node_levelclaims the allowed range is only [0, 63] although other parts of the documentation indicate that it can go up to 127 and in my tests the high values work as well. Should I fix the C++ comment?(star_seedinplayer:set_starsis internally a 64-bit unsigned integer, which I guess is larger than Lua 5.1 can represent safely in integers. I intentionally did not mention a range here so that the safe integer range kicks instar_seedrange now specified as[ulua])Things to test
Although I have proofread my changes myself by using the C++ code as a reference, it is possible that still errors have snuck in. Please compare the documentation updates against the C++ code as well.
Status
This PR is ready for review.
Off-topic: Things to keep in mind for later
IMO the C++ type of the internal stored value after you call(This point is moot, Luanti refuses to compile on systems wheeremeta:set_intshould be switched frominttolongbecause of the de-facto usage of 32-bit integers and the fact the documentation already suggests 32-bit integers. Technically,intonly guarantees 16-bit values in the C++ specintis 16-bit.)math.minintegerandmath.maxintegerto define the safe integer range, so that programmers know exactly which numbers are "safe" to use as integer and at which point integers are "skipped" due to floating-point arithmetic. The namesmath.minintegerandmath.maxintegerare stolen from Lua 5.3. 😉