Skip to content

Fix entity baseline overflow #1427

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

VReaperV
Copy link
Contributor

@VReaperV VReaperV commented Nov 9, 2024

Split net messages if there are too many baselines. I don't like the solution too much because it only works for this exact issue, but I guess it'll do.

The layouts in https://users.unvanquished.net/~reaper/maps/layouts/spacetracks/ can be used to test this with map spacetracks.

Fixes Unvanquished/Unvanquished#2124.

@DolceTriade
Copy link
Contributor

DolceTriade commented Nov 10, 2024

Note that currently, servers can already work around the issue using #1322 which will allow these to load at the cont of high ping players getting a little bit of lag when they see these ents assuming they are in the baseline.

Also see #1321 for some prior art on this topic on why the concept of the baseline is problematic to begin with.

Indeed this might improve the situation, but is it worth it when we have a workaround?

@VReaperV VReaperV force-pushed the reaper/fix-baseline-overflow/sync branch from 363711e to cd9be72 Compare April 14, 2025 07:17
@VReaperV
Copy link
Contributor Author

CI now fails on an unrelated file (#1645 should fix that).

@VReaperV
Copy link
Contributor Author

To boldly go where no man has gone before.

@VReaperV VReaperV merged commit 3dda072 into for-0.56.0/sync Apr 14, 2025
6 of 9 checks passed
@VReaperV VReaperV deleted the reaper/fix-baseline-overflow/sync branch April 14, 2025 08:45
@slipher
Copy link
Member

slipher commented Apr 14, 2025

We are making a point release so this is not going to be included.

@VReaperV
Copy link
Contributor Author

I never said it would, it's targeting the for-0.56.0/sync branch rather than master.

@slipher
Copy link
Member

slipher commented Apr 14, 2025

What's the motivation for rushing to merge it without review then?

@VReaperV
Copy link
Contributor Author

I wouldn't call it rushing given that the pr has existed in its current state for half a year. The only objection to it is similar to the one for map scripting: theoretical bullshit being used as an argument over a small change that works in practice and fixes crashes and situations where a server can be left largely inaccessible. So I'm doing what @illwieckz has said in regards to being bold with merges and not going into the mindset of having to wait for approval for clear improvements (and the theoretical argument presented earlier has so far generated 0 practical value).

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.

3 participants