netfox icon indicating copy to clipboard operation
netfox copied to clipboard

Fix starting order for `RollbackSynchronizer`

Open TheYellowArchitect opened this issue 1 year ago • 1 comments

Solves #260 and #268 by fixing input initialization.

Also gets rid of the await 1 frame hack for 2 seperate classes (BrawlerController.gd/player.gd) in order to await for input authority to be set before process_authority, so this awaiting a frame logic is moved to RollbackSynchronizer since in most games, input authority is set in _ready or something similar.

May also solve the reported problem of the first 2 ticks, having NPCs spinning. Haven't seen it in a project of mine, but it may solve it because this PR addresses the first _earliest_input being set properly

TheYellowArchitect avatar Sep 22 '24 18:09 TheYellowArchitect

EDIT: The below is arguments against and for, the await 1 frame logic, ultimately ending in for keeping things as the code in this PR is.


Btw the "await 1 frame" logic could be skipped entirely. It is only useful if the Input authority is set in the same PackedScene as RollbackSynchronizer. The tree order in Godot is top to bottom. So if the input authority is set in _ready, then that Input node must be above RollbackSynchronizer, so Input's _ready fires before RollbackSynchronizer's.

At forest-brawl, the input authority isn't set at _ready of root brawler or at _ready of Input, but outside, at the spawner:

func _spawn(id: int) -> BrawlerController:
	var avatar = player_scene.instantiate() as BrawlerController
	avatars[id] = avatar
	avatar.name += " #%d" % id
	avatar.player_id = id
	spawn_root.add_child(avatar)
	
	# Avatar is always owned by server
	avatar.set_multiplayer_authority(1)

	print("Spawned avatar %s at %s" % [avatar.name, multiplayer.get_unique_id()])
	
	# Avatar's input object is owned by player
	var input = avatar.find_child("Input")
	if input != null:
		input.set_multiplayer_authority(id)
		print("Set input(%s) ownership to %s" % [input.name, id])
	

So as it is, without the await 1 frame, all _ready trigger, and then input authority is set -> which ofc bugs. Ideally, input authority would be set inside _ready of Input node. I can expand this PR for this, and to remove the await 1 frame, but that would make setting input authority outside the PackedScene impossible, so I suggest keeping this PR as is.

TheYellowArchitect avatar Sep 23 '24 09:09 TheYellowArchitect

Updated PR, turns out the single-frame wait is not needed at all. Also explained it under https://github.com/foxssake/netfox/issues/260#issuecomment-2464755741

elementbound avatar Nov 08 '24 13:11 elementbound

There is a weird "bug" which I have to mention. If a client joins for the first full second, sometimes, the client hovers. Here is a video example of this: https://youtu.be/BZGODoPt_6w I tried removing the deferred call and straight up adding an await, but the result was the same. This shouldn't block the merge, because without this PR there is straight up desync lol

To confirm this bug isn't a regression, I checked out to commit https://github.com/foxssake/netfox/pull/284/commits/20c5bfd97d91e6f279acabcc4000797060636db4 It was very rare compared to this commit, but I did manage to reproduce it at that commit too. Though the "disabled hovering" didn't last as long as latest commit.

I assume it has something to do with is_initialized and should be visible = false until then? idk since is_initialized was added on the latest commits, but this start hover/delay didn't last that long on the previous commit.

P.S. All testing was done with 200ms, and input broadcast disabled.

TheYellowArchitect avatar Nov 13 '24 19:11 TheYellowArchitect

On the addition of is_initialized, since its a boolean, might as well convert it to integer as it was (started_at_tick) to track which tick the rollback synchronizer started. The default value being -1. So the condition comparison is if started_at > -1

TheYellowArchitect avatar Nov 13 '24 19:11 TheYellowArchitect