Skip to content

Option to make sky stars deterministic #16529

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 22, 2025
Merged

Option to make sky stars deterministic #16529

merged 3 commits into from
Nov 22, 2025
+22 −3

Conversation

@fetsorn
Copy link
Contributor

@fetsorn fetsorn commented Sep 29, 2025 •
edited
Loading

Goal of this PR is to make a consistent night sky.

Solves #16521.

The star constellations are now based on a seed passed in StarParams.

This is a patch I play with personally.

Relates to a larger plan for sky layers outlined in #11366.

Ready for Review

How to test

  1. Build
  2. Update your minetest server with the mod https://gitea.your-land.de/fetsorn/consistent_stars. This mod sets the new star_seed option to the world seed.
  3. set "/time 0" then logout and login again. The stars in the sky remain the same. As opposed to being different each login without this patch.

@Zughy Zughy added @ Script API Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Sep 29, 2025
@DragonWrangler1
Copy link
Contributor

DragonWrangler1 commented Sep 29, 2025

Seems to me much better than having them decided randomly based on the player

@appgurueu
Copy link
Contributor

appgurueu commented Sep 30, 2025

Idea for a maybe better design: Add a seed param to player:set_sky, with a missing seed defaulting to current behavior.

Benefits: Can hide map seed from clients, can choose game-specific seed distinct from world seed per player, backwards compat (does it matter?) is kept.

@fetsorn
Copy link
Contributor Author

fetsorn commented Oct 1, 2025

PR is updated with appgurueu's design and ready for review again

appgurueu
appgurueu reviewed Oct 24, 2025
View reviewed changes
appgurueu
appgurueu reviewed Oct 24, 2025
View reviewed changes
@fetsorn
Copy link
Contributor Author

fetsorn commented Oct 29, 2025

PR is updated with appgurueu's recommendations and ready for review again

appgurueu
appgurueu reviewed Oct 29, 2025
View reviewed changes
@fetsorn
Copy link
Contributor Author

fetsorn commented Oct 30, 2025

yes, thank you for your patience with me! updated.

@sfence
Copy link
Contributor

sfence commented Oct 30, 2025

I planned to implement this in the future. So I support it.

sfence
sfence requested changes Oct 30, 2025
View reviewed changes
Copy link
Contributor

@sfence sfence left a comment

Choose a reason for hiding this comment

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

You also need to update l_get_stars function in l_object.cpp file.

It should return the actual value of star_seed, as described in lua_api.md.

@Zughy Zughy added Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand labels Oct 30, 2025
@fetsorn fetsorn requested a review from sfence October 31, 2025 05:19
@sfan5 sfan5 changed the title Make sky stars consistent Option to make sky stars deterministic Oct 31, 2025
@fetsorn
Copy link
Contributor Author

fetsorn commented Nov 9, 2025 •
edited
Loading

updated with sfence's recommendation last week, ready for review 🙏

appgurueu
appgurueu reviewed Nov 9, 2025
View reviewed changes
appgurueu
appgurueu requested changes Nov 9, 2025
View reviewed changes
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

one small thing, otherwise this looks fine :)

sfence
sfence approved these changes Nov 10, 2025
View reviewed changes
Copy link
Contributor

@sfence sfence left a comment

Choose a reason for hiding this comment

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

Looks fine.

@sfence
Copy link
Contributor

sfence commented Nov 10, 2025

Question to consider: Should builtin/game/features.lua be updated for this?

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Nov 11, 2025
@sfan5
Copy link
Collaborator

sfan5 commented Nov 11, 2025

Question to consider: Should builtin/game/features.lua be updated for this?

No, since this is a client feature.

@fetsorn
Copy link
Contributor Author

fetsorn commented Nov 12, 2025

updated setStarSeed as per appgurueu's recommendation. ready for final review!

sfan5
sfan5 requested changes Nov 12, 2025
View reviewed changes
@fetsorn
Copy link
Contributor Author

fetsorn commented Nov 13, 2025

stopped force pushing and squashing until the review is over, cause the lack of history is what confused me in the last commit i think. thank you for your patience, ready for review.

sfan5
sfan5 reviewed Nov 13, 2025
View reviewed changes
@fetsorn
Copy link
Contributor Author

fetsorn commented Nov 18, 2025

not sure how idiomatic my latest change is, but it works. ready for review.

p.s. also added set_stars and set_stars 0 to https://gitea.your-land.de/fetsorn/consistent_stars for testing.

appgurueu
appgurueu reviewed Nov 19, 2025
View reviewed changes
src/client/sky.cpp
void Sky::setStarSeed(u64 star_seed)
{
// Allow force updating star seed at game init.
if (m_star_params.star_seed != star_seed || m_first_update) {
Copy link
Contributor

@appgurueu appgurueu Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nitpick: I don't think the || m_first_update is necessary. setStarCount already calls updateStars() on the first update; if the star_seed is unchanged, there's nothing to do?

Copy link
Contributor Author

@fetsorn fetsorn Nov 21, 2025

Choose a reason for hiding this comment

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

alright! If you say so :) I copied || m_first_update from setStarCount thinking that it's a typical setter, but turns out setStarCount is a special case which I do not completely understand yet. o.0 Does setStarCount check m_first_update because it is called in Sky constructor?

Copy link
Contributor

@appgurueu appgurueu Nov 22, 2025

Choose a reason for hiding this comment

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

Does setStarCount check m_first_update because it is called in Sky constructor?

Basically. Something needs to call updateStars the first time, and setStarCount does that.

This could probably be refactored, but we can do that later.

In either case it's not a problem.

appgurueu
appgurueu approved these changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Tested, works as expected.

I have a nitpick, but it is not blocking.

@appgurueu appgurueu added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Nov 19, 2025
sfan5
sfan5 approved these changes Nov 21, 2025
View reviewed changes
Copy link
Collaborator

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

looks good

@SmallJoker SmallJoker merged commit d959692 into luanti-org:master Nov 22, 2025
16 of 17 checks passed
@appgurueu appgurueu mentioned this pull request Nov 22, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@appgurueu appgurueu appgurueu approved these changes

@sfan5 sfan5 sfan5 approved these changes

@sfence sfence sfence approved these changes

Assignees

@sfence sfence

Labels

Feature ✨ PRs that add or enhance a feature Roadmap: supported by core dev PR not adhering to the roadmap, yet some core dev decided to take care of it @ Script API >= Two approvals ✅ ✅

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fetsorn @DragonWrangler1 @appgurueu @sfence @sfan5 @SmallJoker @Zughy