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

Tick fake players in network updating phase #1847

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

zly2006
Copy link

@zly2006 zly2006 commented Dec 1, 2023

In this PR, I moved fake player logic from EN to NU, so they can be ticked like real players.

There is also a new rule called fakePlayerTicksInEU for players who want the old fake player mechanics, it is false by default.

@Fallen-Breath
Copy link
Contributor

See this discussion on scicraft discord in 2020, for gnembon's opinion on the player action pack timing: https://discord.com/channels/211786369951989762/573613501164159016/771405054632394782

for that to work properly would require a lot of chances and code - essentially attaching client code to the server part. Right now fake players don't differ that much from pigs. Is it bad that they behave like other server side entities?

@Fallen-Breath
Copy link
Contributor

Also, "EN" and "NU" are not commonly used terms in the non-Chinese community. I would suggest using more precise words to describe them

@zly2006
Copy link
Author

zly2006 commented Dec 1, 2023

Also, "EN" and "NU" are not commonly used terms in the non-Chinese community. I would suggest using more precise words to describe them

Could you provide more suggestions? what word should i use for tick phases?

for that to work properly would require a lot of chances and code - essentially attaching client code to the server part. Right now fake players don't differ that much from pigs. Is it bad that they behave like other server side entities?

In fact, in most situations we just expect fake players to be ticked and interact with other things in the game (actions) like real players, so that most farms can work both for real and fake players such as raid farms.

Btw, there is an option to configure--if you want pig-like fake players you can just turn it on.

connection.player.doTick();
// connection.player.absMoveTo(connection.firstGoodX, connection.firstGoodY, connection.firstGoodZ, connection.player.getYRot(), connection.player.getXRot());

// todo: vehicle?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll suggest to mark the PR as a draft, if it's not completed yet

@zly2006 zly2006 marked this pull request as draft December 1, 2023 14:58
desc = "make fake players ticked in entity update phase, like legacy carpet.",
category = CREATIVE
)
public static boolean fakePlayerTicksInEU = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a default value that changes the existing behavior is BAD. It breaks people's contraptions that rely on the old behavior sliently

}
}

@Inject(method = "doTick", at = @At(value = "HEAD"))
Copy link
Contributor

Choose a reason for hiding this comment

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

You are alternating where real player's action pack gets ticked right? then the same as https://github.com/gnembon/fabric-carpet/pull/1847/files#r1412213260

@@ -23,6 +25,11 @@ public void send(final Packet<?> packetIn)
{
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

any explanations for the changes in these files? they don't seem related

import java.util.List;

public class FakePlayerManager {
public static List<ServerGamePacketListenerImpl> connections = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: use concurrency-proof containers to handle potential concurrent access in certain situations (e.g. using mods)

@Fallen-Breath
Copy link
Contributor

Could you provide more suggestions? what word should i use for tick phases?

  • EU: entity phase
  • Network (when ServerConnectionListener#tick gets invoked): just network phase
  • NU (when c2s packets are handled, and async tasks are executed): player phase, or async task phase

In fact, in most situations we just expect fake players to be ticked and interact with other things in the game (actions) like real players, so that most farms can work both for real and fake players such as raid farms.

that's gnembon's opinion, I'll suggest to ask him directly

Btw, there is an option to configure--if you want pig-like fake players you can just turn it on.

See #1847 (comment)

@altrisi
Copy link
Collaborator

altrisi commented Jan 18, 2024

Is there any downside to only ticking them in the right/more correct phase? If there isn't I'd remove the old behaviour completely.

@zly2006
Copy link
Author

zly2006 commented Jan 19, 2024

Is there any downside to only ticking them in the right/more correct phase? If there isn't I'd remove the old behaviour completely.

Some contraptions may be breoken after we changed this, so I think it is a right choice not to cchange the existing behavior and add an option to tick them in the correct phases.

@altrisi
Copy link
Collaborator

altrisi commented Jan 19, 2024

How many contraptions depend on fake players working differently than real ones? (outside of ones built specifically to detect them). I'd expect fake players would be used to test things where real players would be the actual target, so having them work for real-like players would be the idea (that's been the idea of fake players from the start, just wasn't completely accurate).

Having two code paths seems redundant when one is clearly "better" (more accurate). We'd also release this on a snapshot/major (mojang versioning wise) version anyway and we don't usually backport, so the chances of breaking being unnoticed are lower.

@zly2006
Copy link
Author

zly2006 commented Jan 19, 2024

How many contraptions depend on fake players working differently than real ones? (outside of ones built specifically to detect them).

afaik, some raid farms are specially designed for fake players for the tick phase order, this makes them 1gt faster then real players

Having two code paths seems redundant when one is clearly "better" (more accurate). We'd also release this on a snapshot/major (mojang versioning wise) version anyway and we don't usually backport, so the chances of breaking being unnoticed are lower.

Yes, it is a breaking change so I just suggest we shoulld be more careful, I totally agree that a contraption that real players cannot use it not "useful".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants