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.

Sign up for GitHub

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

Remove references to PoolVector #10329

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Remove references to PoolVector #10329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 1 addition & 25 deletions contributing/development/core_and_modules/core_types.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,36 +103,12 @@ which are equivalent to new, delete, new[] and delete[].
memnew/memdelete also use a little C++ magic and notify Objects right
after they are created, and right before they are deleted.

For dynamic memory, the PoolVector<> template is provided. PoolVector is a
standard vector class, and is very similar to vector in the C++ standard library.
To create a PoolVector buffer, use this:

.. code-block:: cpp

PoolVector<int> data;

PoolVector can be accessed using the [] operator and a few helpers exist for this:

.. code-block:: cpp

PoolVector<int>::Read r = data.read()
int someint = r[4]

.. code-block:: cpp

PoolVector<int>::Write w = data.write()
w[4] = 22;

These operations allow fast read/write from PoolVectors and keep it
locked until they go out of scope. However, PoolVectors should be used
for small, dynamic memory operations, as read() and write() are too slow for a
large amount of accesses.
For dynamic memory, use Vector<>.
Copy link
Member

Choose a reason for hiding this comment

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

This page should also mention LocalVector, and Packed*Vector.

  • Vector<> - copy-on-write and can be safely passed via public API.
  • Packed*Vector - aliases for specific Vector<*> types accessible via GDScript.
  • LocalVector<> - non COW version, with less overhead, for the internal use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that this PR is fixing a years-old documentation bug. The page should mention these, but is mentioning these a hard enough requirement that it should block merging the bugfix PR? That is, is it acceptable for the page to say only "For dynamic memory, use Vector<>" while a separate PR is worked on, that documents the alternatives?

Copy link
Member

@bruvzg bruvzg Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it is acceptable as a bug fix. For dynamic memory, use Vector<> is correct, and in general a safe option to use, so it's OK as is.

LocalVector<> is a more advanced option with more limitations, and can be added in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's OK as a bugfix, then let's merge this one as-is.

@esainane, you're welcome to document the other options mentioned here if you want to, as another PR, after this one is merged. If you don't feel knowledgeable enough, that is also OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I should be able to do something proper this Sunday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a suitable follow up ready to go, in case people were waiting?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to bundle it into this PR, since it's not actually merged yet.

(I thought we might have merged this already, but without an explicit second approval from an experienced engine dev I ended up not merging it yet)


References:
~~~~~~~~~~~

- `core/os/memory.h <https://github.com/godotengine/godot/blob/master/core/os/memory.h>`__
- `core/pool_vector.h <https://github.com/godotengine/godot/blob/master/core/pool_vector.cpp>`__

Containers
----------
Expand Down
4 changes: 2 additions & 2 deletions tutorials/navigation/navigation_using_navigationlayers.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ In scripts the following helper functions can be used to work with the ``navigat
var path_query_navigation_layers: int = 0
path_query_navigation_layers = enable_bitmask_inx(path_query_navigation_layers, 2)
# get a path that only considers 2-nd layer regions
var path: PoolVector2Array = NavigationServer2D.map_get_path(
var path: PackedVector2Array = NavigationServer2D.map_get_path(
map,
start_position,
target_position,
Expand Down Expand Up @@ -120,7 +120,7 @@ In scripts the following helper functions can be used to work with the ``navigat
var path_query_navigation_layers: int = 0
path_query_navigation_layers = enable_bitmask_inx(path_query_navigation_layers, 2)
# get a path that only considers 2-nd layer regions
var path: PoolVector3Array = NavigationServer3D.map_get_path(
var path: PackedVector3Array = NavigationServer3D.map_get_path(
map,
start_position,
target_position,
Expand Down