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

Gate disabling of fog or camera update behind 'debug' priv #14539

Merged
merged 1 commit into from May 5, 2024

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Apr 12, 2024

fixes #14163

"camera update" stops updating the render list depending on the camera position, which allows you to see through block edges (and sometimes the world disappears entirely). This is basically a wallhack.

also unbinds both keys by default

@sfan5 sfan5 added this to the 5.9.0 milestone Apr 12, 2024
@sfan5 sfan5 force-pushed the fogcamup branch 2 times, most recently from 40bda1c to 032e032 Compare April 17, 2024 13:51
builtin/settingtypes.txt Outdated Show resolved Hide resolved
@grorp
Copy link
Member

grorp commented May 3, 2024

Tested with Shadow Forest. While this fixes the biggest problem, there's still something weird remaining: Whether unlimited viewing range is enabled or not seems to have an effect on culling despite unlimited viewing range being "forbidden".

screenshot 1

screenshot 2

@sfan5
Copy link
Member Author

sfan5 commented May 3, 2024

Whether unlimited viewing range is enabled or not seems to have an effect on culling despite unlimited viewing range being "forbidden".

draw_control->range_all contains the state wanted by the user. The code that sets up fog additionally checks sky->getFogDistance() < 0 to take the server restriction into account, all other code doesn't.
This is quite messy and should be separated into a bool for the wanted and one for the actual state.

@sfan5 sfan5 merged commit d748c8c into minetest:master May 5, 2024
13 checks passed
@sfan5 sfan5 deleted the fogcamup branch May 5, 2024 12:26
@Montandalar
Copy link
Contributor

I think that hiding fog should only require the BASIC_DEBUG HUD flag. Camera Update is an xray/wallhack though so I agree with that going behind the debug priv. Here is my rationale for using the HUD Flag:

I think this should have been done in some way that doesn't break the existing expectation in many existing games that fog is toggleable to ordinary users and not just a debug setting. I would have much preferred if mods could enable like how the mapping kit was added to Minetest Game after removing the default ability to open the minimap.

I get that Minetest Game isn't the default any more, where the expectation exists; but disabling fog just makes the view so much clearer without needing really high render distances or infinite view range, both of which can bog the framerate down when all you want is a clearer view.

When I wrote #14163 (comment) I specifically mentioned mods rather than the debug privilege. I liked how the mapping kit was a HUD flag so I could add a mod to enable or disable it. What I don't like is the idea of giving out the debug privilege just so users can see without fog.

@sfan5
Copy link
Member Author

sfan5 commented May 6, 2024

I think the amount of existing games that expect the user to disable fog is tiny at best. (Also you can't even use this key on Android.)

Besides with fog_start in the set_sky API it should be possible to effectively disable fog in the proper server-controlled way.

@Montandalar
Copy link
Contributor

Well, yes I suppose it is then possible to do something like add an inventory formspec button that disables fog for the client. That's not quite as seamless as before, but it does technically allow client control after asking the server.

And it would actually support Android now that you mention it. In some version of the future where more keybinds are possible it could possibly become as easy as it used to be again.

Okay, I am convinced that at least it is still user-configurable, just with more faff. And it outlines the need for an intercompatibility mod for setting parts of the sky. But the functionality is still there.

@Sokomine
Copy link
Contributor

Sokomine commented May 9, 2024

sfan5 wrote:

I think the amount of existing games that expect the user to disable fog is tiny at best.

The amount/relevance of existing games that require forbidding disabling of fog is tiny. There exists at least one example as shown in the screenshots here. But that's not the bread-and-butter use case of Minetest. Such games are the new variant, while beeing able to disable fog was the up-to-now standard.

In general, disabling fog ought to be the choice of the player - same as view range. Most servers and games will not care. The player's hardware and internet connection and in part the servers' capabilities limit what's possible here.

On all servers and games that I tried so far view range could be increased to high values. So where's the point of forced fog? Having view range reduced to 10 (because the most limited view range of 20 is all that works in a certain situation due to other bugs in the game or in mods) is no fun. Especially not if you can have it at higher values with more potent hardware. The argument of cheating does not hold.

(Also you can't even use this key on Android.)

Ok, that's a problem.

Besides with fog_start in the set_sky API it should be possible to effectively disable fog in the proper server-controlled way.

So...each server out there, each game - has to change its settings if someone with a new client connects? How realistic is that? Sure, the big servers will eventually manage. Their admins will have a lot of extra work so that nobody can "cheat" in a few new games out there. And all the old games need to be changed.

I've checked those servers that I'm registered on and enjoyed playing (and still value). Plus my local development game (MTG + varying mods). Result: The server Tunnelers' Abyss was the only server where there was no problem and fog could be enabled/disabled. All other servers and games I tested showed the following behaviour: Fog was enforced. The server owners and games most likely know nothing of their "luck" yet. View range was not limited. At least I could turn it up until hardware and internet connection gave up (several 100m).

I'm afraid this setting in its current form does far more collateral dammage than it does any good.

Or is it just a bug? I couldn't find any calls to set_sky in MTG. Yet such a newly created test world enforced fog.

The default definitely ought to be to allow players to choose if they want fog or not.

@sfan5
Copy link
Member Author

sfan5 commented May 9, 2024

The basis for this change is my belief that disabling fog was a mere feature someone thought made sense during the MT 0.3 days and that somehow made it unscathed during the 0.4/5.0 transition.

I think the focus on games that use this (or don't) is a bit misleading.
The point as also stated in #14163 is that we have an API that allows to move the fog really close to the player and restrict the view to e.g. 15 nodes but it effectively doesn't work because the player can just press F3 to (mostly) bypass it.
MTG does not have any feature that uses this but something simple as a weather mod might want to make use of this, so "MTG is most popular so disabling fog is fine in most cases" would be missing the point.

If disabling fog is to be considered a gameplay feature a compromise I can see is to allow disabling fog as long as the view range is also unrestricted (range_all also already works like this).

Or is it just a bug? I couldn't find any calls to set_sky in MTG. Yet such a newly created test world enforced fog.

No, that's working as designed. With this PR you can't disable fog by default.

There exists at least one example as shown in the screenshots here. But that's not the bread-and-butter use case of Minetest. Such games are the new variant, while beeing able to disable fog was the up-to-now standard.

FWIW I don't think that's an argument in itself. Minetest is trying to become more of an engine where the game author has precise control over the display/rendering and the user can't just change their settings and the artistic vision goes out the window.

@Sokomine
Copy link
Contributor

Sokomine commented May 9, 2024

sfan5 wrote:

If disabling fog is to be considered a gameplay feature a compromise I can see is to allow disabling fog as long as the view range is also unrestricted (range_all also already works like this).

That'd make some sense.

No, that's working as designed. With this PR you can't disable fog by default.

For me, this is a breaking change. There are games and servers and perhaps mods out there that may want to forbid disabling the fog - ok. But they are new and few compared to all that exists already. The default behaviour in such a case ought to be to keep the previous behaviour and allow those who want to change/restrict it to the new behaviour.

Even the documentation implies that servers/games that want the old behavior don't have to change anything. lua_api.md says about set_sky:
* Passing no arguments resets the sky to its default values.
And regarding fog_start:
*fog_start: float, override the client's fog_start. Fraction of the visible distance at which fog starts to be rendered. By default, fog_start is controlled by the client's fog_start setting, and this field is not set. Any value between [0.0, 0.99] set the fog_start as a fraction of the viewing_range. Any value < 0, resets the behavior to being client-controlled. (default: -1)
This reads to me as if doing nothing specific (using default values) is the safe way to let the client decide about fog. Perhaps that requires an empty call to set_sky() with no parameters? That's a bit uncertain.

sfan5 wrote:

FWIW I don't think that's an argument in itself. Minetest is trying to become more of an engine where the game author has precise control over the display/rendering and the user can't just change their settings and the artistic vision goes out the window.

Giving the games more control can in some situations be justified. Some servers definitely want more control over some things. But please make that optional! Make it a real choice - don't entforce it on players and server owners who may not even have heard about it yet and don't want it!

The "artistic vision going out of the window" is extremly thin ice I'm afraid. For example my buildings on servers are built with the servers' default textures. Now what if a player installs another texture pack? The building may look very diffrent, far from how it was intended, and even rather bad. My artistic vision is violated. Now...shall players be enforced not to install texture packs? I don't think I have the right to force them to that. All I say is that if they use another texture pack it may not look as intended and I take no responsibility for it then looking bad. It's the players' choice, not mine. And besides texture packs...most monitors (including mine) display colors...somehow. Only few people have expensive calibrated ones. And some people are even color blind. They have no chance to see the build as it was intended.

Right now, fog is forced on us that don't want it and didn't use it in the past. That is a violation of artistic vision.

And even if you're excited about a new game (which is understandable) which needs a new feature (namely: not beeing able to disable fog) - please think about the future. What'll happen when the next game comes up with a new idea about how default behaviour has to be changed for everything existing? Then this currently new game will no longer look the way it was intended.

Does it cost that much effort to keep the old normal behaviour (client decides) as default and just allow all those that need a new behaviour to make that choice in their games?

@SmallJoker
Copy link
Member

SmallJoker commented May 9, 2024

A possible compromise that I can think of while reading the conversation above:

  1. Gate fog when fog_distance and/or fog_start are specified by a PlayerRef:set_sky({fog = { ... } }) call.
  2. Keep the debug privilege to exempt players from this rule (if that API is used at all by games)
  3. Same rules for camera update (EDIT: dead braincells)

This way, the behaviour would not change for games/servers that do not care about the fog settings. And if they did - they'd have to use the set_sky API to specify a value that they think is appropriate for the gameplay experience.

I can understand that some people might have suboptimal vision, in which case I think it should be up to the game to provide assistance options of any sort to players who need it.

@grorp
Copy link
Member

grorp commented May 9, 2024

Your compromise suggestion is effectively the same as sfan5's above.

Same rules for camera update

You don't suggest gating "Camera Update" behind the set_sky fog API, do you? From what I can see, everyone so far agreed with gating it behind the "debug" priv.

sfan5 added a commit to sfan5/minetest that referenced this pull request May 9, 2024
@sfan5
Copy link
Member Author

sfan5 commented May 9, 2024

implemented the proposed compromise: #14634

@Sokomine
Copy link
Contributor

sfan5 wrote:

implemented the proposed compromise: #14634

Thanks a lot! That will definitely help and solve the issue.

sfan5 wrote in that pull request (but more relevant to this discussion here so I'll comment here):

IMO disabling fog is still stupid even visually, compare these two:

Ah. Your screenshots at least help me to see why you think so and didn't consider not beeing able to disable fog any issue.

The view range has an extreme effect in this case. If you can afford a sufficiently high view range, having the "end" of the world fogged out a bit makes it look visually more pleasant. That's right and feels so for me as well.

One workaround for my problems (apart from switching to an older release) was to increase view range to twice of what I usually use. That helped in beeing able to continue to play - to a degree. There are reasons for decreasing the view range. In those cases where I could increase it, my computers' fan turned up and got audibly louder - decidedly unpleasant but acceptable for a short time. Also, power consumption increases considerably. In those situations where I couldn't increase view range due to technical limitations, the server admin had to hand me out the debug priv so that I could continue playing. Not perfect either and not a solution for all.

Trouble starts if you're forced to go down to minimal view range. A city on a clear summer day turns into something foggy like that level from Shadow Forrest. It becomes difficult to navigate, and there's just not enough left to see. If one were to drive with a car in such a situation it'd be down to driving at walking pace.

While at the same time the Shadow Forrest level could be played in clear view if the view distance could be increased (good for the level that it's also limited there).

Based on my observations I think that an absolute percentage of fog feels unrealistic. Fog on short view range is much more intense than the same value on far view range. That's not so in RL. If your view range is blocked by something nearby you might not even notice that there's fog.

If someone ever wants to implement this more fully, shorter view range probably requires a smaller fog value than higher view range. But I don't think that's a very urgent or important issue. This is just a comment if someone ever searches for this.

Thanks again for fixing!

sfan5 added a commit to sfan5/minetest that referenced this pull request May 12, 2024
sfan5 added a commit that referenced this pull request May 14, 2024
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.

Fog can be disabled (API-sent as well)
5 participants