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

Misc. docs and renames for niche ECS internals #16786

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

JaySpruce
Copy link
Contributor

Objective

Some structs and methods in the ECS internals have names that don't describe their purpose very well, and sometimes don't have docs either.

Also, the function remove_bundle_from_archetype is a counterpart to BundleInfo::add_bundle_to_archetype, but isn't a method and is in a different file.

Solution

  • Renamed the following structs and added docs:
Before After
AddBundle ArchetypeAfterBundleInsert
InsertBundleResult ArchetypeMoveType
  • Renamed the following methods:
Before After
Edges::get_add_bundle Edges::get_archetype_after_bundle_insert
Edges::insert_add_bundle Edges::cache_archetype_after_bundle_insert
Edges::get_remove_bundle Edges::get_archetype_after_bundle_remove
Edges::insert_remove_bundle Edges::cache_archetype_after_bundle_remove
Edges::get_take_bundle Edges::get_archetype_after_bundle_take
Edges::insert_take_bundle Edges::cache_archetype_after_bundle_take
  • Moved remove_bundle_from_archetype from world/entity_ref.rs to BundleInfo. I left the function in entity_ref in the first commit for comparison, look there for the diff of comments and whatnot.
  • Tidied up docs:
    • General grammar and spacing.
    • Made the usage of "insert" and "add" more consistent.
    • Removed references to information that isn't there.
  • Renamed BundleInfo::add_bundle_to_archetype to BundleInfo::insert_bundle_into_archetype for consistency.

@NthTensor NthTensor added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Dec 12, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Really nice set of changes. AddBundle in particular has really confused and aggravated me in the past. I agree with all of these changes, and while the type names are verbose, they're both arcane and internal so I think it's a fine tradeoff.

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

changes make sense to me.

pub(crate) struct AddBundle {
/// The target archetype after the bundle is added to the source archetype
/// Used in [`Edges`] to cache the result of inserting a bundle into the source archetype.
pub(crate) struct ArchetypeAfterBundleInsert {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it might be better to name this and other methods ArchetypeAfterInsertBundle and *_insert_bundle() to match the insert_bundle ordering.

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 went back and forth on this, but I stuck with bundle_insert because I thought insert_bundle might sound too much like an "action", like get_archetype_after_insert_bundle would actually be doing the insert itself. Not married to it though

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really have too strong feelings here either and I don't think it's worth thinking about too hard. The new names are better, so lets just ship it.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 12, 2024
Merged via the queue into bevyengine:main with commit d132239 Dec 12, 2024
35 checks passed
@JaySpruce JaySpruce deleted the tidy_ecs_internals branch December 13, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants