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

Register.lua: Throw error if node 'light_source' > core.LIGHT_MAX #4523

Closed
wants to merge 1 commit into from
Closed

Register.lua: Throw error if node 'light_source' > core.LIGHT_MAX #4523

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Sep 15, 2016

Add 'core.LIGHT_MAX = 14' to builtin/game/constants.lua with the intention
to replace misplaced 'default.LIGHT_MAX = 14' in Minetest Game.
Add comment in light.h requiring the constant be changed in both places.
Add lighting bug warning to note in lua_api.txt.
There are hundreds of mod uses of 15 which causes a lighting bug.
///////////////////////////////////////////////////

More thorough fix for #3964 than #4519
See #3964 (comment)
An error halts the game and notifies which node caused the error.

@kwolekr
Copy link
Contributor

kwolekr commented Sep 15, 2016

Is the light source maximum documented?
Should 14 be a constant instead?

@paramat
Copy link
Contributor Author

paramat commented Sep 16, 2016

'default.LIGHT_MAX = 14' is set in MTGame, perhaps this should be added to builtin/game/constants? Although it seems weird because it's unlikely to change more than once every few years. Anyway i don't mind doing this.
Might add to the note in lua_api.txt that 15 causes a lighting bug.

@rubenwardy
Copy link
Member

LIGHT_MAX should be builtin IMO, it's odd that it's a default feature

@paramat
Copy link
Contributor Author

paramat commented Sep 16, 2016

I agree, i'm adding it to builtin constants.

@paramat paramat changed the title Register.lua: Throw error if node 'light_source' > 14 Register.lua: Throw error if node 'light_source' > core.LIGHT_MAX Sep 16, 2016
@paramat
Copy link
Contributor Author

paramat commented Sep 16, 2016

Updated. Now we can use core.LIGHT_MAX and slowly replace default.LIGHT_MAX.

@@ -20,3 +20,6 @@ core.EMERGE_GENERATED = 4

-- constants.h
core.MAP_BLOCKSIZE = 16

-- Maximum value for node 'light_source' parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's from light.h, which could be noted here.

ShadowNinja's comment about noting it in the header file as well, can apply here too.
c8b4bed#commitcomment-18510949

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@paramat
Copy link
Contributor Author

paramat commented Sep 17, 2016

Updated with request.

light_source = 0, -- Amount of light emitted by node (max. 14)
light_source = 0, --[[
^ Amount of light emitted by node (max. 14).
^ A value of 15 causes a lighting bug. ]]
Copy link
Contributor

@kwolekr kwolekr Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to reword this documentation to use core.LIGHT_MAX.

I also think it's odd to point out the currently observed behavior when the value is out of range in a piece of official documentation. If anything at all, this should read "The effect of values outside the range of 0 through core.LIGHT_MAX is undefined"

Copy link
Contributor Author

@paramat paramat Sep 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, but decided it was clearer to modders to mention the actual current values, as they will probably not be familiar with these constants as set in core code. They want to be able to instantly see the actual values to use in a mod. Luckily these values will not change often.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, but it should still be made known that core.LIGHT_MAX is the constant to use for maximum light. We certainly don't want modders to get in the habit of hard coding numeric literals for things that might change in a future version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah i see, will edit.

@paramat
Copy link
Contributor Author

paramat commented Sep 17, 2016

Updated lua_api.txt comments.

Add 'core.LIGHT_MAX = 14' to builtin/game/constants.lua with the intention
to replace misplaced 'default.LIGHT_MAX = 14' in Minetest Game.
Add comment in light.h requiring the constant be changed in both places.
Add lighting bug warning to note in lua_api.txt.
There are hundreds of mod uses of 15 which causes a lighting bug.
@kwolekr
Copy link
Contributor

kwolekr commented Sep 17, 2016

👍

@paramat
Copy link
Contributor Author

paramat commented Sep 17, 2016

3aefa5d

@paramat paramat closed this Sep 17, 2016
@paramat paramat mentioned this pull request Sep 17, 2016
@paramat paramat deleted the limitlight2 branch September 23, 2016 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants