Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Documentation #3

Merged
merged 4 commits into from
May 11, 2020
Merged

Documentation #3

merged 4 commits into from
May 11, 2020

Conversation

Defman
Copy link
Member

@Defman Defman commented May 11, 2020

This PR adds documentation to all publicly exposed functions.
This PR also adds fecs::World::batch_remove, Legion does not expose legion::World::add_components to allow for fecs::World::batch_add.

A few notes reading through the source.

  1. Executor::set_up should only be run once or should we keep track of which systems have been set up correctly? This can be easily be done by tracking the length at each call to Executor::set_up since there's no way of removing systems.
  2. EntityBuilder::add should not be exposed publicly since EntityBuilder::with should be preferred?
  3. EntityBuilder::build_one seems strange and useless since spawning the entity into a world resets the builder, it does however reuse the internal vectors?
  4. World::try_get should be renamed World::get and World::get should be removed in favour of Option::unwrap?

@Defman
Copy link
Member Author

Defman commented May 11, 2020

Legion refactoring renames a lot of functions to be more in line with the standard library, should we do the same?

  1. Rename World::spawn -> World::extend.
  2. Add World::push or World::insert for a singel entity.
  3. Rename World::is_alive -> World::contains.
  4. Rename EntityBuilder::spawn_in -> EntityBuilder::push_into or EntityBuilder::insert_into

If we add World::push it makes the EntityBuilder redundant or is it still useful to delegate the entity creation across multiple functions?

fn foo(builder: &mut EntityBuilder) {
    builder.add(...)
}

let mut builder = EntityBuilder::new()
    .with(...);

foo(&mut builder);

builder.build().spawn_in(&mut world);

src/builder.rs Outdated
pub fn build(self) -> BuiltEntity<'static> {
BuiltEntity {
builder: CowMut::Owned(self),
written: false,
}
}

/// This one seems redudent, since spawning clears the internal components.
Copy link
Member

Choose a reason for hiding this comment

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

build_one can be used to re-use the EntityBuilder's allocations as an optimization.

src/events.rs Outdated
@@ -82,7 +82,7 @@ impl EventHandlers {
}
}

/// Triggers an event.
/// Emeits the given event `E` with given resources and world.
Copy link
Member

Choose a reason for hiding this comment

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

Typo

@caelunshun
Copy link
Member

@Defman my plan is to keep naming consistent with hecs. We could switch to Legion's naming, but that would cost a bunch of refactoring in Feather which serves no real purpose.

@caelunshun
Copy link
Member

Thanks!

@caelunshun caelunshun merged commit 98da464 into feather-rs:master May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants