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

[ecs] Entry API for components and try_insert command #2418

Closed

Conversation

NathanSWard
Copy link
Contributor

@NathanSWard NathanSWard commented Jun 30, 2021

Objective

Solution

  • Adds a component function to EntityMut
  • Adds a try_insert function to EntityCommands.

TODO:

  • Add examples of entry API

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 30, 2021
@NathanSWard NathanSWard added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jun 30, 2021
@MinerSebas
Copy link
Contributor

What is this doing differently compared to #2061?

@NathanSWard
Copy link
Contributor Author

What is this doing differently compared to #2061?

The primary discourse around #2061 and #2054 was the try_insert_bundle and what its purpose was.
IMO, try_insert for Components does not carry the same baggage and this PR limits the scope of the issues.

@mockersf
Copy link
Member

I think the name try_insert is wrong. In Rust, try_ methods are variant of a panicking method that can't panic. Here it doesn't change anything to the panicking possibilities...

But I don't have any meaningful suggestion for better naming.

@NathanSWard
Copy link
Contributor Author

I think the name try_insert is wrong.

Yep, I had this thought as well.
I think ideally we'd want some like the HashMap::entry API, however that feels like a little too much right now.
std::HashMap does have a try_insert method (albeit unstable) here.

@Nilirad
Copy link
Contributor

Nilirad commented Jul 1, 2021

insert_checked
insert_if_absent
insert_unique
insert_dedup

@mockersf
Copy link
Member

mockersf commented Jul 1, 2021

for completion in IDE, it would be nice to start the name with insert

@NathanSWard
Copy link
Contributor Author

Would we want something similar to the HashMap::entry API?

enum ComponentEntry {
    Vacant(..),
    Occupied(..),
}

world.entity_mut(e).component::<T>()

@wilk10
Copy link
Contributor

wilk10 commented Jul 2, 2021

insert_checked
insert_if_absent
insert_unique
insert_dedup

i really like insert_if_absent, it's the most literal / unambiguous, imo

@Nilirad
Copy link
Contributor

Nilirad commented Jul 2, 2021

i really like insert_if_absent, it's the most literal / unambiguous, imo

I have to admit though that having the word if in the identifier does not look the best.

@NathanSWard NathanSWard changed the title [ecs] Add a try_insert for components. [ecs] Add an Entry API for components. Jul 2, 2021
@NathanSWard
Copy link
Contributor Author

NathanSWard commented Jul 2, 2021

@mockersf @Nilirad @wilk10
I updated the current implement to have a HashMap::entry like API.
I'm curious to see what your thoughts are?

Note: EntityCommands still has a try_insert function.
However, EntityMut has a new entry API.

@NathanSWard
Copy link
Contributor Author

In Rust, try_ methods are variant of a panicking method that can't panic

The more I think about this, the more I don't think its a problem.
Just because a method has try_ in front of it doesn't necessarily mean the non try_ variant will panic.

Especially in the context of inserting something for an already valid entity, IMO, this is quite clear.

I think the best example of try_ functions where the non-try variant doesn't panic is the proposed

  • std::HashMap::try_insert here
  • hashbrowh::HashMap::try_insert here

World and entity->component relationship ois very similar to the HashMap key->value relationship.

@NathanSWard NathanSWard changed the title [ecs] Add an Entry API for components. [ecs] Entry API for components and try_insert command Jul 2, 2021
@wilk10
Copy link
Contributor

wilk10 commented Jul 3, 2021

Thanks a lot for your work!

I can get behind this reasoning and the two other examples linked make a good case for it. I also don't think it changes the meaning of insert. It conveys very well that the component will be inserted unless some condition is true.

insert_if_absent goes further and specifies the reason why it won't insert it, but i agree that the presence of if is not great, and maybe users will use it for all insertions, also the first one?
The other proposals were not immediately super obvious to me.

We do lose out on IDE completion aid, but i guess it's an acceptable trade-off.

@NathanSWard
Copy link
Contributor Author

We do lose out on IDE completion aid, but i guess it's an acceptable trade-off.

This isn't quite true (at least as a VSCode/Rust Analyzer user.
If you type in insert things that start with insert will pop up first, but also, functions that contain that contain the name insert will also pop up.
So the function doesn't necessarily have to start with insert.

@mockersf
Copy link
Member

mockersf commented Jul 4, 2021

The try_* that you linked (andtry_from) returns a Result which is not possible here as the command execution is asynchronous.

It bothers me to use this word when neither the function name or the return type can explain why it could fail / if it failed

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@NathanSWard
Copy link
Contributor Author

Closing for now since there is probably a design discussion to be had to better this API

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants