-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Proposal: API to control shadow intensity from the game/mod #11944
Conversation
The API is designed to be used with weather mods, allowing to change visual appearance of the sky and shadow intensity in one network packet. |
I am highly sceptical that this belongs in What is the rationale to expand that function instead of making a |
I'd put it inside |
The sky has the sun (or not), it makes sense to me to control it from there.
Update: Apparently I am tired. Sky is also per player. Still seems right to attach it to the thing that controls sun and clouds. |
I'd put the lua command behind the debug (or some other) priv, though. NM: It's devtest only. |
@erlehmann @kilbith I was considering a separate api call for controlling the shadows, and went with set_sky because both the color of the sky, clouds and shadow intensity are part of atmospheric ambience, and clouds are already in set_sky. I see a much stronger use case for shadows to be changed with the weather (sky and clouds) than with, say moon phase or sun texture. As for orbit tilt, that clearly belongs to the sky bodies (and it could even be made different for sun and moon) |
#11499 proposes |
I like this solution. The question for me really only is the API design.
player:set_lighting()? |
As long as we are talking about shadows cast by the sun (or moon) I would still prefer this to be part of the sky API, it's also what is currently controlling the angle of the shadow. |
It's a matter of how you want to plan on grouping stuff in the future. set_sky() is fine to me. |
I don't think this is the right solution for 5.5.0.
With this PR there's also the question of what the default shadow intensity would be, it needs to be zero so that the feature is entirely opt-in. If preserving the shadow feature for 5.5 is a priority then I can envision a solution that allow games to opt-in via their |
@sfan5 but this is enough to allow a maintained game to disable shadows very easily (which satisfies the requirements @hecktest came up with), and there's nothing in this that would make it difficult to extend this to the features you mention in later versions. Right? Of course if you go with the strict compatibility definition of "no shadows unless the game explicitly requests them", then the default should be 0. I can think of another (maybe more serious) problem: The scale of shadow intensity doesn't go to 100% which would be something along the lines of "completely black" (or actually that'd be "completely ambient brightness" (see #11499 for what that'll probably work and look like in the future)), which a game might want to do. So this doesn't give the kind of total control we should aim for. |
Sure it is an opt-out solution but is that what hecks requested? Short of asking him again there's no sure way, going by his hard stance on "pulling the rug" I think an opt-out is better. Regardless of the opt-in/opt-out decision, I think an explicitly temporary way is better than adding an API that will inevitably need changes because it won't match the final idea of what the API should be (see also #11366) |
@sfan5 @celeron55 A strict opt-in solution will prevent people from trying shadows on older servers until the server owners upgrade and use the new API, which in turn would mean a client upgrade for their entire user base. The solution here is a tradeoff between user and game control, allowing the users to opt-in shadows on older games and servers if they see fit and newer/maintained games to opt-out or take (some) control of the shadows as they want. As for giving the game the full control (up to 100%), might be a good idea. I make |
Hecks has said a few times that he's fine with updating his game after an engine change. I support defaulting with allowing shadows |
MHO, this is right compromise. We can (and have) endlessly discuss this; perhaps this should be controlled by the Biome, or perhaps by the player, or the gamedev, or ..., or ... Arrghhh. It's just shadows for chrissake! :) At this rate we'll never add anything new to Minetest. @0xLiso has already left us (probably due to the tone of a some folks who will remain unnamed). With this the client can set it, game can override if they care. We have already proposed turning it off by default on the client, so players would have to actively turn it on, and then presumably they want it. As usually, just my $0.02. With @SmallJoker's concern addressed I fully support this. |
@celeron55 wHy does @hecktest specify the requirements regarding shadows being opt-in or not? Other games, like the MCL games, have light/shadow mechanics too! Is it because @hecktest has a game which is affected very badly? Some games simply do not have mechanics where a sun-cast shadow makes little sense. |
Tried it. Works. Code looks good. |
@erlehmann It is pretty much just because @hecktest formulated a general rule that seems to make sense to me. He's one of the people who care about the issue because he has a game with a very specific art style. Some others don't have a similar reason to care about it. You can propose another general rule if you'd like. Have you mentioned what your take on opt-in/opt-out would be? @x2048's "allowing the users to opt-in shadows on older games and servers if they see fit" seems to make sense to me also, though. |
So, it looks like we have some agreement around opt-out behavior, but there is doubt about (ab-)using set_sky. Is I will expand shadow_intensity to the full range and rename I will keep orbit inclination with |
Usage of the new API (to enable a new feature on new clients) does not mean incompatibility with old clients. Anyway I've stated my opinion on this already. Don't forget the last sentence in my comment, which applies regardless of opt-in or opt-out! |
As long as we have an API, it's cool. :) |
What does that mean? It's no longer multiplied by the client setting? So if the game does not set it the client setting is used, if the games sets it the client setting is ignored? |
@lhofhansl yes |
Create shadows namespace for all future shadow-specific parameters
Sorry... Missed the update here. Will review and test today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it. Works. Code looks fine.
I would like to accompany that with a change in MTG that sets the shadow strength based on cloud coverage. There is already code to set the cloud density based on biome humidity, it's just 3 lines of code there.
Added One Approval label based on @lhofhansl's approval. |
Should we add things like saturation et all later? |
@lhofhansl Yes, I'll add more stuff to set_lighting in later PRs. |
Is the API in this PR currently opt-in or opt-out? |
@sfan5 Here is how I see the API for shadows, and the current API is within this design:
|
While we're at it. I still prefer an opt-out for games that care about this. But I take this over nothing. |
(ah, downvote-nocomment-@wsor4035 has graced us with his vote again :) ) |
my opinion has not changed from #11944 (comment) |
Sounds good, I have no complaints about that. I might have time to review this soon but if not feel free to merge it with someone else's approval. |
I've added support for sun/moon being hidden. If you approve 3677ddd, I'll merge this PR. |
So uhh this is stinking great. Now I have to manually add code to enable shadows, which isn't a huge deal, but it breaks the server for anybody that is using the stable release. Effectively this makes shadows completely useless. |
Why does it break the server for anyone on a stable release? |
@sfan5 When joining a the server with a client running the stable version the client would crash because it didn't know how to handle player:get_lighting() BuckarooBanzai came through with a solution on the discord. |
Please be more specific. The client does not handle Lua calls, it's all server side. |
This might be a compatibility-workaround as sfan5 said for multiple engine-version support but i think your problem with the crashes in the clients (?!) should not happen like that, do you have that installed on a public server for easier testing by any chance? (this is getting OT though, might be the time to open a separate issue) |
So I added the code that was included in the merge, I think the bit from devtest. Singleplayer crashed on stable, dev was fine. Now that I think about it I didn't push the changes to my server, so maybe that actually wasn't/isn't a problem. |
My original comment is incorrect, I apologize for that. I was irked that this was made opt-in and posted too hastily, I should have done better testing.
to a minetest.register_on_joinplayer() callback on a dev server doesn't crash stable clients, they obviously see no shadows. A dev client joining the dev server does see shadows. Running a dev client after this commit effectively nukes shadows on all games/servers, unless the server is running a dev build or the game has the compatibility code. How is anybody suppose to do any testing of the shadows when no games/servers support them? As far as I can tell neither MTG or DevTest have the shadows enabled. |
@NathanSalapat As you can see from my various comments and suggestions I wholeheartedly agree with you. Of all things that you can set on the client to break things or change gameplay or emergence it seems to be shadows that has been elected by most of the core-devs to be the one things that breaks gameplay. (And why let a user decide what (s)he sees with their own client when we can also make it all really complicated, just to serve the one or two one-off games that do not want shadows. Oh well.) On this, note however, that dev test already has a chat command to turn on shadows: "set_lighting" |
This is a proposal to fix #11365 and #11972.
Specification
Lua API
A game should be able to control intensity of ambient shadows as part of a new set_lighting() API call
set_lighting() API call will also be used for ambient brightness (#11499) and saturation (#11854). A matching get_lighting() API call will be used to retrieve the current values of the lighting and shadows parameters.
Network
set_lighting call translates into a new TOCLIENT_SET_LIGHTING network packet containing parameters packed one after another.
Client
Introduce new setting
shadow_intensity_gamma
to control the visual darkness of shadow intensity values to accommodate for differences between computer screens.The formula is: actual_intensity = (input_intensity) * (1 / shadow_intensity_gamma).
When actual_intensity is zero:
To do
This PR is Ready for Review.
Follow-up tasks:
How to test
shadow_strength_gamma
parameter/set_lighting 0
- shadows disappear, and FPS goes up/set_lighting 0.5
- shadows appear at half intensity/set_lighting 1
- shadows appear at full intensity (black)Check
games/devtest/mods/experimental/lighting.lua
for the example code.Note
Deleting and recreating the SM textures at runtime causes irrlicht errors and an eventual crash, won't fix in this PR.
Edit: spelling, add a todo