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

Allow to set maximum star opacity at daytime #11663

Merged
merged 1 commit into from Jul 2, 2022

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Sep 29, 2021

Fixes #9887. Successor of #10541.

This PR adds a new Lua API feature to player:set_stars: day_opacity. This is a floating point number between 0.0 and 1.0. It sets the maximum opacity of stars at daytime, with 1.0 meaning fullbright stars (same as night) and 0.0 being fully invisible.

Technically, the mechanism to do this is very simple: A value of X just means the star opacity is capped at X at daytime, meaning it cannot get more opaque during the day.

The default value is 0.0 and this feature is backwards-compatible. I've tested a 5.5.0-dev client on a 5.4.0 server and it worked.

Note: The word "opacity" here means the opacity of the stars BEFORE star_color is applied. This means "full opacity" here means that the full effect color of star_color is applied, it does not neccessarily mean that the stars are actually fully opaque. Stars never get more opaque than the alpha value of the star_color, like before. This is NOT new behavior, I just pointed that out for clarity.

To do

Ready for review.

How to test

Function test in 5.5.0-dev:

  • Activate the luacmd mod
  • Start DevTest
  • /lua me:set_stars({star_color="#FF0000FF"}) (this is optional, but red stars are better for testing; also note the default star_color is not fully opaque, so don't be surprised if day_opacity=1 doesn't make the stars fully opaque when using the default star_color)
  • /time 12000
  • Check sky (expected: no stars)
  • /lua me:set_stars({day_opacity=1})
  • Check sky (expected: visible stars)
  • /lua me:set_stars({day_opacity=0})
  • Check sky again (expected: no stars)
  • /time 0
  • Check sky again (expected: visible stars)
  • /lua me:set_stars({day_opacity=1})
  • Check sky again (expected: visible stars)
  • /time 12000
  • /lua me:set_stars({day_opacity=0.75})
  • Check sky again (expected: reduced opacity stars)

Feel free to try out other opacity values.
Also try to let a full day/night circle complete (under various conditions) to check whether the transition is still smooth in all cases. You could do /set time_speed 300 to speed things up.

Compability test:

  • Launch a 5.4.1 server
  • Connect with your compiled -dev client to it
  • Watch the sky at night and at day; for compability reasons, the stars should behave in the default way (visible at night, not visible at day)

@ghost
Copy link

ghost commented Feb 23, 2022

Looks promising from a quick test, but haven't given this a full pass yet.

That said, this needs to be rebased and fixed to work with master. This is missing changes to compile with the latest IrrlichtMt, and if we rebase to get them the code no longer compiles without modification due to #11983. The fix is fairly trivial though, it's a 1-liner.

@ghost ghost added the Rebase needed The PR needs to be rebased by its author. label Feb 23, 2022
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Feb 27, 2022

The rebase is complete.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Feb 28, 2022
@ghost
Copy link

ghost commented Mar 1, 2022

Just a quick note--I saw the notification, and will be reviewing this weekend. I'm taking a break from reviews this week since I'm participating in a game jam, but this is at the top of my list and will probably get looked at again on Saturday.

@ghost
Copy link

ghost commented Mar 5, 2022

Tested, seems to work as intended.

  • Singleplayer - no issues
  • New server, old client - changing old client's daytime opacity does nothing, as expected
  • Old server, new client - obviously impossible to change daytime opacity, both players see the same star opacity as before

I did see one odd thing--at time speed 720 (2 minute cycle), the stars 'warp' to new positions around sunrise. At time speed 72 though, this did not occur. It could be an existing behavior that wasn't visible before, I'm not sure if we want to investigate it now or not worry about it for the time being.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

One other thing I'd like to raise before approving: Should we have night_opacity or some variant thereof? It may sound redundant at a glance, but I suspect we can't presently make daytime stars brighter than nighttime ones even with this change.

I'm not sure how useful this feature is, but it seems like it could be worthwhile to have the option available for the sake of flexibility. Any opinions from others would be appreciated.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Mar 15, 2022

I did see one odd thing--at time speed 720 (2 minute cycle), the stars 'warp' to new positions around sunrise. At time speed 72 though, this did not occur. It could be an existing behavior that wasn't visible before, I'm not sure if we want to investigate it now or not worry about it for the time being.

This does not seem to happen reliably, tho, only sometimes. And it's also the sun and moon that suddenly "jump". So I think this must be an old, unrelated issue. Won't touch.

One other thing I'd like to raise before approving: Should we have night_opacity or some variant thereof? It may sound redundant at a glance, but I suspect we can't presently make daytime stars brighter than nighttime ones even with this change.

Won't do. I don't see a need right now. But if it will be needed, there is always chance for another PR.

@rubenwardy rubenwardy added Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels Apr 25, 2022
@rubenwardy
Copy link
Member

tested

@rubenwardy rubenwardy merged commit 142928e into minetest:master Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set_sky: Show stars (if enabled) at all times in "plain" and "skybox" modes (maybe optionally?)
3 participants