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

Remove shadow_sky_body_orbit_tilt setting with the next major release #13707

Closed
wants to merge 21 commits into from

Conversation

Zughy
Copy link
Member

@Zughy Zughy commented Jul 31, 2023

Partly fixes #13705 , it needs to disappear from settings as well (@rubenwardy ?)
See #13705 (comment)

To do

This PR is Ready for Review.

How to test

  1. Stand on one leg
  2. tilt your head by 57° to the right
  3. start jumping
  4. read

@grorp
Copy link
Member

grorp commented Sep 11, 2023

shadow_sky_body_orbit_tilt is a pure client-side setting, so removing it would be no compatibility breakage. You can hardcode a default value of 0.0f for body_orbit_tilt instead of reading the default value from shadow_sky_body_orbit_tilt.

The setting was reintroduced to fix an uninitalized value on the client in some cases. See this comment by x2048:

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.

Originally posted by x2048 in #13340 (comment)

It should cause no problems to have a hardcoded default value instead.

@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 12, 2023
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 24, 2023
@Zughy
Copy link
Member Author

Zughy commented Sep 24, 2023

@grorp I hope it's correct

@grorp
Copy link
Member

grorp commented Sep 25, 2023

This is what I meant. In theory, this should be backwards compatible, except for singleplayer-only games with a shadow_sky_body_orbit_tilt setting in their game-specific minetest.conf.

Going one step further, we could replace the nil server-side default value with a default value of 0.0 as well, but that could break mods which do something like this:

if not player:get_sky().body_orbit_tilt then
   player:set_sky({ body_orbit_tilt = 60 })
end

Anyway, this PR needs a zipgrep. And probably it should get a review by x2048 when you're ready.

@rubenwardy
Copy link
Member

Searched in .lua and .conf files:

image

@Zughy
Copy link
Member Author

Zughy commented Sep 30, 2023

@x2048 thoughts?

@SmallJoker
Copy link
Member

SmallJoker commented Oct 1, 2023

I don't see how such minor change in settings would need any special treatment. The functionality is not lost. Simply remove all traces of shadow_sky_body_orbit_tilt (looking at doc/breakages.md) and merge this thing. No need to make a big fuss about it.

@grorp
Copy link
Member

grorp commented Oct 1, 2023

@rubenwardy Could you do a zipgrep for "body_orbit_tilt" as well? Then we would know whether we can also remove the special nil logic for body_orbit_tilt in set_sky.

(EDIT: With special nil logic, I mean the INVALID_SKYBOX_TILT thing. I didn't see SmallJoker's comment before writing this.)

@rubenwardy
Copy link
Member

@rubenwardy Could you do a zipgrep for "body_orbit_tilt" as well?

This is also not present in any .lua or .conf files

@x2048
Copy link
Contributor

x2048 commented Oct 1, 2023

@Zughy I guess you'll have to decide it without me: https://irc.minetest.net/minetest-dev/2023-10-01#i_6119219

@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 4, 2023
@rubenwardy
Copy link
Member

what action/change does this need?

@grorp
Copy link
Member

grorp commented Oct 8, 2023

what action/change does this need?

Removing the INVALID_SKYBOX_TILT thing as well, it only exists because of the setting.

But I'd also be fine with just removing the setting and keeping the INVALID_SKYBOX_TILT thing.

grorp and others added 4 commits October 8, 2023 21:44
* settingstypes.txt: Fix wrong default value for profiler.report_path

* Disable Irrlicht file picker on Android
  (It doesn't work.)

* Join Game tab: Fix server description textarea being misaligned with background

* Reduce distance between tab and gamebar on Android
  Allows using a higher gui_scaling value without the gamebar going off-screen.

Co-authored-by: ROllerozxa <rollerozxa@voxelmanip.se>
…13686)

Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>
@Zughy
Copy link
Member Author

Zughy commented Oct 8, 2023

...and I've fucked up my git history

@Zughy
Copy link
Member Author

Zughy commented Oct 8, 2023

Well, let's cut the chase: this PR was initially just a documentation PR. It's clear I have no idea of what I'm doing now and that grorp is babysitting me. Considering also the current git history, I simply invite anyone to take over, as I feel we're all wasting our time at the moment. Closing and labelling for adoption

@Zughy Zughy closed this Oct 8, 2023
@Zughy Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Adoption needed The pull request needs someone to adopt it. Adoption welcomed! @ Script API @ Startup / Config / Util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove shadow_sky_body_orbit_tilt setting