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.

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

Server enforcement for fog_distance (#13448) to block cheating #13643

Merged
merged 3 commits into from
Jul 6, 2023
Merged

Server enforcement for fog_distance (#13448) to block cheating #13643

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e1b4ad
Limit server set fog_distance to [0,4000] when set
lhofhansl Jul 2, 2023
edeee2a
Enforce fog_distance at server to block cheating
lhofhansl Jul 2, 2023
64ea6b3
Revert "Limit server set fog_distance to [0,4000] when set"
lhofhansl Jul 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/clientiface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,12 @@ void RemoteClient::GetNextBlocks (
s32 new_nearest_unsent_d = -1;

// Get view range and camera fov (radians) from the client
s16 fog_distance = sao->getPlayer()->getSkyParams().fog_distance;
s16 wanted_range = sao->getWantedRange() + 1;
if (fog_distance >= 0) {
// enforce if limited by mod
wanted_range = std::min<unsigned>(wanted_range, std::ceil((float)fog_distance / MAP_BLOCKSIZE));
}
float camera_fov = sao->getFov();

/*
Expand Down
4 changes: 4 additions & 0 deletions src/script/lua_api/l_object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1806,6 +1806,10 @@ int ObjectRef::l_set_sky(lua_State *L)
lua_getfield(L, 2, "fog");
if (lua_istable(L, -1)) {
sky_params.fog_distance = getintfield_default(L, -1, "fog_distance", sky_params.fog_distance);
if (sky_params.fog_distance >= 0) {
// if set, enforce game limits
sky_params.fog_distance = rangelim(sky_params.fog_distance, 20, 4000);
}
Copy link
Member

@grorp grorp Jul 3, 2023 •
edited

Choose a reason for hiding this comment

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

I think the minimum is too high. A fog distance of 10 allows for really nice effects, but there could also be use cases for something like 2. The only value I really see no use for is 0.

I don't think we should define an artificial minimum here. Instead, we could just change this

Any value >= 0 sets the desired upper bound for the client's viewing_range and disables range_all.

into this

Any value > 0 sets the desired upper bound for the client's viewing_range and disables range_all.

and call it a day. (Not only in the docs, but also in the implementation, of course.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client won't allow less than 20.

minetest/src/client/game.cpp

Line 2513 in 25ef8f3

if (range_new < 20) {

Copy link
Member

@grorp grorp Jul 3, 2023 •
edited

Choose a reason for hiding this comment

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

The client won't allow the user to set a value below 20, but via the current set_sky API, you can set values below 20 and they will be applied. E.g. this works:

minetest.register_on_joinplayer(function(player)
    player:set_sky({ fog = { fog_distance = 2 }})
end)

And I think there's no good reason to disallow it.

Copy link
Contributor Author

@lhofhansl lhofhansl Jul 3, 2023 •
edited

Choose a reason for hiding this comment

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

I do not feel strongly. The main part was to avoid was allowing setting this to 0 - which would not render anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get some consensus. I can change this to only disallow 0 as valid value.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see an issue with setting fog_distance even to 0. Why do we need a limit?

Copy link
Contributor Author

@lhofhansl lhofhansl Jul 3, 2023 •
edited

Choose a reason for hiding this comment

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

At 0 you see nothing. Even the wield item is not shaded correctly.
It's not just black (or fog color) it looks like a bug.

Actually: Can you try and see what you think (without this PR)?

Copy link
Member

@grorp grorp Jul 4, 2023 •
edited

Choose a reason for hiding this comment

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

At 0 you see nothing. Even the wield item is not shaded correctly.
It's not just black (or fog color) it looks like a bug.

Some more testing reveals that for me, any fog_distance value below 1 results in only the fog color being visible, both with shaders and with the fixed pipeline. The value of fog_start doesn't seem to make any difference.

-- for Minetest game
minetest.register_on_joinplayer(function(player)
    minetest.after(0, function()
        player:set_sky({ fog = { fog_distance = <insert value here>, fog_start = 0.5 }, type = "plain", base_color = "#ff0000", clouds = false })
    end)
end)

All screenshots were taken from the same point of view. The first screenshot is always with shaders (enable_shaders = true) and the second with the fixed pipeline (enable_shaders = false). The differences are probably (partially) due to the fact that tone mapping is enabled (tone_mapping = true).

fog_distance = 10 screenshot with fog_distance set to 10, shaders enabled screenshot with fog_distance set to 10, shaders disabled
fog_distance = 1 screenshot with fog_distance set to 1, shaders enabled screenshot with fog_distance set to 1, shaders disabled
fog_distance = 0.999999 screenshot with fog_distance set to 0.999999, shaders enabled screenshot with fog_distance set to 0.999999, shaders disabled
fog_distance = 0.1 screenshot with fog_distance set to 0.1, shaders enabled screenshot with fog_distance set to 0.1, shaders disabled
fog_distance = 0 screenshot with fog_distance set to 0, shaders enabled screenshot with fog_distance set to 0, shaders disabled

So, something seems to break with values below 1.

Copy link
Member

Choose a reason for hiding this comment

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

fog_distance is an s16, that's why it's rounded.

Not seeing anything bug fog is the intended behaviour for fog_distance=0, I'd say.
And the wieldhand issues are not that severe, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok... Make this simpler :)
I'll post an update soon.

sky_params.fog_start = getfloatfield_default(L, -1, "fog_start", sky_params.fog_start);
}
} else {
Expand Down