-
Notifications
You must be signed in to change notification settings - Fork 13
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
Introducing scenarios concept #153
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a spin and not too much to say other than what is in the description, few minor comments scattered around.
I did however run into a major issue, where it seems project loading / saving is broken, specifically by running the demo scenario, saving it, then trying to load it, loading failed with the following error:
2023-07-25T04:39:46.735174Z ERROR librmf_site_editor::workspace: Failed loading site Error { code: ExpectedIdentifier, position: Position { line: 254, col: 14 } }
Which specifically points to the first location:
43: {"anchor": 42, "charger": Some(""), "parking": Some(""), "holding": Some(""), "name": "tinyRobot1_charger", "graphs": All},
There is also a flaw in this PR where robots will become invisible when their parent anchor gets hidden. This will be fixed in the future after we migrate to the latest version of Bevy.
This might require a bit more thought about the parenthood. It is true that there will be a "always visible" variant for the Visibility
component, which ignores hierarchy's visibility. However, since we use the hierarchy visibility to show / hide levels, the moment we set an entity to always visible this will be ignored and we might see all the robots in other levels in the current view.
#[cfg_attr(feature = "bevy", derive(Component))] | ||
pub struct LocationTags { | ||
#[serde(default, skip_serializing_if = "Option::is_none")] | ||
pub charger: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does String
contain (since it was bool before)? Is it meant to be something like a json
that will be sent to the robot, or just a human readable description? This could also be "documented" in the widget through hover text or a label. +100 for the change to a LocationTags
struct since the Vec<LocationTag>
Component made tag search and update a bit painful before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still deliberating this. For now it's a placeholder meant for us to experiment with. My current expectation is it can be a json
containing information about what type of charger it is. E.g.
{
"brand": "Xnergy",
"height_range": [0.1, 2.0],
"current": 2.0,
"voltage": 15
}
then there can be application logic somewhere to decide which robots are compatible with the charger.
This PR introduces the concept of scenarios and begins moving the implementation of "models" in a new direction:
This PR leaves the data structures in a limbo where
Model
still exists to refer to furnishings that are laid out around the levels. I left those as-is to minimize how disruptive this PR is. They will also be migrated into the new scenario concept after the existing PRs are merged in and there's less risk of merge conflicts. They will probably be referred to asDecor
orFurnishings
.There is also a flaw in this PR where robots will become invisible when their parent anchor gets hidden. This will be fixed in the future after we migrate to the latest version of Bevy.