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

Return shadow_sky_body_orbit_tilt setting #13340

Merged
merged 4 commits into from Mar 24, 2023
Merged

Conversation

x2048
Copy link
Contributor

@x2048 x2048 commented Mar 19, 2023

Used as a default value when the game does not change the value via API (e.g. legacy server)

Fixes #13302.

To do

This PR is Ready for Review.

How to test

  1. Set the setting shadow_sky_body_orbit_tilt = 60
  2. Enable shadows
  3. Start a 5.7.0 game with shadows, notice the sun orbit tilt is 60 degrees
  4. Connect to a 5.6.1 server, notice sun orbit tilt is 60 degrees
  5. Add a mod with the following code:
minetest.register_on_joinplayer(function(player)
	player:set_sky({body_orbit_tilt = 30})
end)
  1. Enable the mod and start the game
  2. Notice the orbit tilt is 30 degrees

Used as a default value when the game does not change the value via API (e.g. legacy server)
@x2048 x2048 added this to the 5.7.0 milestone Mar 19, 2023
@x2048
Copy link
Contributor Author

x2048 commented Mar 19, 2023

@Desour the problem was in not having a default value in SkyboxParams when deserialization fails of skips the value. I've returned the setting to act as a drop-in value in these cases.

@sfan5
Copy link
Member

sfan5 commented Mar 19, 2023

Are we sure we want to introduce a setting that only works on older server (this is mostly opaque to the user and will cause confusion)? Why not just hardcode 60 as default?

@x2048
Copy link
Contributor Author

x2048 commented Mar 19, 2023

It will also work for games/servers that don't change orbit_tilt via API.

src/client/sky.h Outdated Show resolved Hide resolved
@lhofhansl
Copy link
Contributor

Looks good (minus the warning)

src/skyparams.h Outdated
@@ -19,6 +19,8 @@ with this program; if not, write to the Free Software Foundation, Inc.,

#pragma once

#include <cmath>
Copy link
Member

Choose a reason for hiding this comment

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

looks unused

@sfan5
Copy link
Member

sfan5 commented Mar 19, 2023

It will also work for games/servers that don't change orbit_tilt via API.

I see. Didn't review the code prior to commenting.
Will this work correctly with get_lighting() (so that it does not return -1024)?

@x2048
Copy link
Contributor Author

x2048 commented Mar 19, 2023

I've added an if to avoid returning orbit_tilt from get_sky(true) when the value is 1024. It is not possible for the modders to set that value because of rangelim in l_set_sky

@sfan5
Copy link
Member

sfan5 commented Mar 19, 2023

should be documented in lua_api.txt that the field can be nil.

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 20, 2023
@x2048 x2048 removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 21, 2023
@x2048
Copy link
Contributor Author

x2048 commented Mar 21, 2023

Expanded the documentation for the parameter, explaining the default behavior and changing the values.

@sfan5 sfan5 merged commit f3b198e into minetest:master Mar 24, 2023
13 checks passed
@x2048 x2048 deleted the orbit_tilt branch March 24, 2023 19:44
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.

MT 5.7 - Skybox broken when using setters from mods
3 participants