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

[MP] Fix division by zero in network profiler #96464

Merged

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Sep 2, 2024

The debugger reports synchronizers with empty state to the editor even if no data is being sent to other peers.

The editor conditional to avoid division by zero was checking the wrong variable.

Fixes #96354 .

Draft status:

  • As reported in [MP] Fix division by zero crash #96450 we might want to show 0 for outgoing sync too, to clarify that no data is being set (although the synchronizer is still being processed).:
    I'll create a proposal for a network profiler revamp to make the information clearer and include "On Change" states, but I think we should merge this as a crash fix in the meantime.

The debugger reports synchronizers with empty state to the editor
even if no data is being sent to other peers.

The editor conditional to avoid division by zero was checking the wrong
variable.
@Faless Faless added bug topic:editor topic:network crash cherrypick:4.1 cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 2, 2024
@Faless Faless added this to the 4.4 milestone Sep 2, 2024
@Faless Faless marked this pull request as draft September 2, 2024 11:54
@Withaust
Copy link
Contributor

Withaust commented Sep 2, 2024

Just tested this fix, it seems to be working great now! But I now have one question: @Faless, how am I supposed to debug the actual send size, if it is constantly reported as being zero? This small MRP that I have provided was ported directly from my game, where I have the exact same scenario with a variable value changes and "On Change" send rate. Since this value is replicated (even if this does not happen every frame), is it possible to somehow understand what the actual send size is?

@Faless
Copy link
Collaborator Author

Faless commented Sep 2, 2024

how am I supposed to debug the actual send size, if it is constantly reported as being zero?

Currently, if it the state size is reported as zero, it means no data is being sent for state synchronization ("Always" variables).

I believe the network profiler has never been updated to include delta synchronization ("On Change" variables).

I'll create a proper proposal for a network profiler revamp to make the information clearer and include "On Change" states, but I think we should merge this as a crash fix in the meantime.

@Faless Faless marked this pull request as ready for review September 2, 2024 14:27
@Withaust
Copy link
Contributor

Withaust commented Sep 2, 2024

I'll create a proper proposal for a network profiler revamp to make the information clearer and include "On Change" states, but I think we should merge this as a crash fix in the meantime.

Yeah, I agree, merging this first should be a priority.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM and matches code above

@akien-mga akien-mga merged commit 7c38376 into godotengine:master Sep 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@Faless Faless deleted the mp/fix_profiler_divsion_by_zero branch September 2, 2024 18:16
@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
Ryan-000 pushed a commit to Ryan-000/godot that referenced this pull request Nov 29, 2024
…ion_by_zero

[MP] Fix division by zero in network profiler
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MP] MultiplayerSynchronizer crashes networking profiler
4 participants