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

Sneak: Add option for old move code #5519

Closed
wants to merge 1 commit into from

Conversation

Projects
10 participants
@paramat
Copy link
Member

commented Apr 5, 2017

Temporary option for classic sneak behaviour.
Enabled by setting the added 'new move' physics override to false.
By default 'new move' is true.
///////////////////////////////////////

Tested.

Temporary (until 0.5 or the end of next release) option that enables the old move code (exactly as it was before new sneak). Keeps players happy while we continue to improve the new move code.

New sneak works differently and so does new sneak ladder and any 2-node jump we might code. Sneak behaviour identical to before is very unlikely, however, worlds have been built that depend on the specific previous behaviour. So i realised that although new sneak is improving, players will not be happy (in the short-term future) with anything other than old move code exactly as it was.

I and others would of course not be happy with old move code being kept for the long-term future.
This PR would give us 6+ months to improve new sneak code, hopefully having the new code available will help players get used to it.
MT then 0.5 gives us a good excuse to remove the old move code and break some parkour worlds.

It seems making the setting a physics override is best, as this gives server admin the choice of either choosing the setting for a world, or allowing players to choose through a chat command.
This would also be consistent with the related 'sneak glitch' override.

@nerzhul
Copy link
Member

left a comment

Why do you duplicate so many code, please factorize maximum

Map *map = &env->getMap();
INodeDefManager *nodemgr = m_client->ndef();

v3f position = getPosition();

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 5, 2017

Member

const v3f &

This comment has been minimized.

Copy link
@Zeno-

Zeno- Apr 6, 2017

Contributor

Can't be const... it's modified by more than one line below (see e.g. line 179, 273 etc)

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 7, 2017

Member

nice shot i missed it


// Copy parent position if local player is attached
if(isAttached)
{

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 5, 2017

Member

brace


// If in liquid, the threshold of coming out is at higher y
if (in_liquid)
{

This comment has been minimized.

Copy link
@nerzhul

nerzhul Apr 5, 2017

Member

many braces

@paramat paramat force-pushed the paramat:sneakoption branch from 0634c46 to 27e6a4d Apr 5, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

Will work on code style.
This is a separate block of code to allow free development of new move code.

@paramat paramat force-pushed the paramat:sneakoption branch from 27e6a4d to c8b85ca Apr 5, 2017

@@ -56,6 +56,7 @@ void set_default_settings(Settings *settings)
settings->setDefault("curl_verify_cert", "true");
settings->setDefault("enable_remote_media_server", "true");
settings->setDefault("enable_client_modding", "false");
settings->setDefault("classic_sneak", "false");

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 5, 2017

Member

this should definitely not be a client side setting

This comment has been minimized.

Copy link
@paramat

paramat Apr 8, 2017

Author Member

Done.

@@ -137,6 +139,8 @@ class LocalPlayer : public Player
v3f m_position;

v3s16 m_sneak_node;
// Stores the max player uplift by m_sneak_node

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 5, 2017

Member

needs a note that this is only used for "classic" sneak

This comment has been minimized.

Copy link
@paramat

paramat Apr 8, 2017

Author Member

Done.

// Enabled by the 'classic_sneak' option (disabled by default).
void LocalPlayer::classic_move(f32 dtime, Environment *env, f32 pos_max_d,
std::vector<CollisionInfo> *collision_info)
{

This comment has been minimized.

Copy link
@sfan5

sfan5 Apr 5, 2017

Member

the code duplication here is very horrible

also you will need a note that any changes to the movement code need to be done in both functions

This comment has been minimized.

Copy link
@paramat

paramat Apr 8, 2017

Author Member

The duplication is intentional.
The old code does not necessarily need improvement, it's for plaeyers who want the old code unchanged.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

👎 That's not how this should be solved. Either revert to the previous (perhaps slightly tweaked) sneak code or fix the new issues of the current one. This just ends up in code duplication and long going discussions about which one is better.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

About code duplication, it's intentional, i know this can be coded with less duplication, but that then makes the new move code more difficult to work on. We do not know what parts of new move code will be altered, so will often have to be rewriting the code that switches between old and new.

I assumed that because sfan5 is working on new move code a lot he will not want to have the job made more complex.
The duplication also makes the old code easier to remove if it is temporary.

How do i make this setting serverside? I agree it probably should be.

Either revert to the previous (perhaps slightly tweaked) sneak code or fix the new issues of the current one.

If we revert while working on the new move code, that means far less players testing the new move code, while also disappointing the many players who prefer the new move code.
Many devs strongly oppose a revert, even c55 has stated it's not really an option.
If we don't include this option, you will then be continuing to disappoint a huge number of players, this is a very big issue, bigger than i expected.

This just ends up in code duplication and long going discussions about which one is better.

That will happen anyway, and the controversy will be much more intense and angry if an option is not available.
SmallJoker can you explain a major disadvantage to having this option? I still can't see one. The code is not as compact as it could be, but for good reason, and some more lines is a minor issue.

What this does is keep players happy and buys us time while we consider what to do. It also allows us to have widespread testing of new move code while also having the old available, helping us to improve the new code faster.

I do hope this will be temporary, hopefully new sneak can be improved to the point where we can remove the old code. But currently, new sneak does not satisfy many players and still avoids damage in some situations, it still needs a long period of work.

@Megaf

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

👍 For the idea/concept. Not sure about the code.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

I've changed the wording to express this will certainly be temporary, probably until the end of next release, maybe co-inciding with 0.5 as a good point to break old stuff.

I would not be happy with old move code being kept for the long-term future. This would give us 6+ months to improve new sneak code, hopefully we can improve the sneak ladder/sneak-jump options so that they are acceptable to many players, and hopefully having the new code available will help players get used to it.

If we can't keep players happy after that, then 0.5 gives us a good excuse to remove the old move code and break parkour worlds.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

About the setting, talking to celeron55 it seems making the setting a physics override is best, as this gives server admin the choice of either choosing the setting for a world, or allowing players to choose through a chat command.
This would also be consistent with the related 'sneak glitch' override. Will work on this.

@paramat paramat force-pushed the paramat:sneakoption branch 2 times, most recently from 104d752 to 7681e42 Apr 6, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

Updated with requested comments and improved comments stating this is optional.
Moved option to a new physics override 'old move'.

@paramat paramat changed the title Sneak: Add option for classic move code Sneak: Add option for old move code Apr 6, 2017

@paramat paramat force-pushed the paramat:sneakoption branch from 7681e42 to 0829d7b Apr 6, 2017

@shivajiva101

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2017

Whilst there is transition it's a sensible option given the amount of server assets created using the old behaviour and this provides a window to define, code and tune parkour relevant behaviour in the new sneak code

@Ezhh

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

An option for this would be welcome and very appreciated.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

What this does is keep players happy and buys us time while we consider what to do. It also allows us to have widespread testing of new move code while also having the old available, helping us to improve the new code faster.

@paramat My disagreement is based on the presumption that we would continue adding alternative code for controversial features. This is fine to enable and disable the sneak glitch but the whole sneak code was just a too big area to add a setting there.
Sorry for the excessive comment up there. Perhaps I've just overseen the part that it is considered to be a temporary option until a solution has been found. Taking back my thumb down reaction. I'm ok with this idea as long it won't stay this way over years.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

I recently added more comments making clear this will certainly be temporary, when you saw this i was saying this is 'possibly temporary', so i can understand your earlier opinion and somewhat agree with it.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2017

To enable old move code fo all players use a mod with this:

minetest.register_on_joinplayer(function(player)
	player:set_physics_override({old_move = true})
end)

To give players the choice a mod can be used that allows players to use chat commands to set the physics override.

@Brackston

This comment has been minimized.

Copy link

commented Apr 7, 2017

Thank you paramat, I am sure many players will be very happy with this option as we wait for the new code. I appreciate your efforts in this matter

The duplication is intentional, see thread comments.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2017

Still need to add docs, will do closer to merge once i get more approval.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2017

Have a problem, i am using this code to set old_move to true on joinplayer:

minetest.register_on_joinplayer(function(player)
	player:set_physics_override({old_move = true})

	if player:get_physics_override().old_move == true then
		print("old_move = true")
	elseif player:get_physics_override().old_move == false then
		print("old_move = false")
	end
end)

But it prints that old_move is false (the default).
Investigating.

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2017

Might be something to do with:

 		// these are sent inverted so we get true when the server sends nothing
 		bool sneak = !readU8(is);
 		bool sneak_glitch = !readU8(is);
+		// this is not sent inverted to get false when the server sends nothing
+		bool old_move = readU8(is);

 	// these are sent inverted so we get true when the server sends nothing
 	writeU8(os, !sneak);
 	writeU8(os, !sneak_glitch);
+	// this is not inverted to get false when the server sends nothing
+	writeU8(os, old_move);

I might have to invert the setting to make the default true?

@paramat paramat force-pushed the paramat:sneakoption branch from 24e2e12 to a578301 Apr 8, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2017

Tested and working, get_physics_override() also working correctly.

@paramat paramat added this to the 0.4.16 milestone Apr 9, 2017

@paramat paramat removed the Controversial label Apr 9, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2017

@paramat paramat removed the WIP label Apr 9, 2017

@paramat paramat force-pushed the paramat:sneakoption branch from a578301 to c9c9aed Apr 9, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2017

I moved the 'old_move()' code block to the end of the file for less disruption and a simpler diff.

@paramat paramat added the Feature label Apr 9, 2017

@paramat paramat force-pushed the paramat:sneakoption branch from c9c9aed to 05bbeb4 Apr 11, 2017

@paramat paramat referenced this pull request Apr 13, 2017

Closed

Sneak: Strip down version #5533

8 of 9 tasks complete

@paramat paramat added One approval and removed One approval labels Apr 15, 2017

@celeron55

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

+1 for concept and protocol

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2017

c55 is ok with code too http://irc.minetest.net/minetest-dev/2017-04-15#i_4876726 so will consider this a 👍

@nerzhul

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

👎 for code duplication, there is no reason to duplicate code in any case

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2017

Nerzhul the duplication is intentional because this is temporary code, please read the first post and #5519 (comment)
Discussion in IRC http://irc.minetest.net/minetest-dev/2017-04-15#i_4877310

@paramat paramat force-pushed the paramat:sneakoption branch from 05bbeb4 to ebc3c26 Apr 17, 2017

Sneak: Add option for old move code
Temporary option for the old move code for specific old sneak behaviour.
Enabled by setting the added 'new move' physics override to false.
By default 'new move' is true.

@paramat paramat force-pushed the paramat:sneakoption branch from ebc3c26 to 27711b8 Apr 17, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

Added docs and merged f6da7b3

@paramat paramat closed this Apr 17, 2017

@paramat

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

To enable old move code for all players use a mod containing this:

minetest.register_on_joinplayer(function(player)
	local override_table = player:get_physics_override()
	override_table.new_move = false
	player:set_physics_override(override_table)
end)

Alternatively you can write a mod to set move code per-player controlled by player chat command.

@paramat paramat deleted the paramat:sneakoption branch Apr 19, 2017

@nerzhul nerzhul added this to Done in Minetest 0.4.16 Apr 29, 2017

@paramat paramat removed this from Done in Minetest 0.4.16 May 18, 2017

@nerzhul nerzhul added this to Done in Minetest 0.4.16 May 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.