Skip to content

[3.x] PoolVector to Vector #108134

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

Draft
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

lawnjelly
Copy link
Member

Just an interesting diversion this morning, I remembered that we do have quite a few outstanding PoolVector bugs, and wondered how easy it would be to just make PoolVector a wrapper around Vector (as they both offer COW).

Some PoolVector bugs e.g.:
#69804
#51852
#53665
#37154

Turns out, for a basic version, it wasn't super tricky. So I'm making a draft here just for reference in future should be decide to go down that route.

There's probably quite a few flaws here as it's quick and dirty. In particular the writes are pretty inefficient because it will call the COW stuff each element is accessed (and the whole point of the Write is to avoid this), and we'd want them to reference the COW data to prevent de-allocation and dangling pointers (which the old version also suffers from). But it's a proof of concept.

Notes

  • I did some basic benchmarking (fps in games, startup times for editor) and it's mostly around the same as vanilla Godot, maybe a little faster, but nothing to get worked up around.
  • The main reason for making such a change would be to remove some of the PoolVector bugs, but otoh it is as likely that this would introduce regressions as much as solving existing bugs 😁 , which is why I'm just adding this as a draft rather than going full steam on it.
  • 4.x notably just got rid of PoolVector altogether and replaced cases by Vector. That's probably something we could do in places in 3.x, but we'd need a PoolVector wrapper for backward compatibility I suspect, especially with bound languages.

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.

1 participant