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

Fix starting order for RollbackSynchronizer #284

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

TheYellowArchitect
Copy link
Contributor

@TheYellowArchitect TheYellowArchitect commented Sep 22, 2024

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

@@ -24,6 +26,7 @@ var _states: Dictionary = {}
var _inputs: Dictionary = {}
var _latest_state: int = -1
var _earliest_input: int
var _started_at_tick: int = -1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable ultimately ended up being unused, but its useful to keep so as to know when an avatar joined/started.


if (await_first_frame):
await get_tree().process_frame
process_authority()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After await, it is basically guaranteed the input authority is set, so process_authority can be safely invoked

@@ -220,6 +232,8 @@ func _get_history(buffer: Dictionary, tick: int) -> Dictionary:
var latest = buffer.keys().max()

if tick < earliest:
_logger.warning("Tried to load tick %s which is earlier than the earliest we have recorded (%s)
Try increasing the history limit." % [tick, earliest])
Copy link
Contributor Author

@TheYellowArchitect TheYellowArchitect Sep 22, 2024

Choose a reason for hiding this comment

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

Debug comment, to confirm #260 is fixed. Place this message on main branch, and you will see it goes from 0 to current. I will remove this message once this PR is reviewed/approved

@@ -47,10 +47,6 @@ func _ready():

_snap_to_spawn()

# TODO: What if the RollbackSynchronizer had a flag for this?
# Wait a frame so Input has time to get its authority set
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And that's what I did :P

@TheYellowArchitect
Copy link
Contributor Author

TheYellowArchitect commented Sep 23, 2024

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.

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.

2 participants