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

New StaticObject API #107

Merged
merged 8 commits into from
Apr 1, 2021
Merged

New StaticObject API #107

merged 8 commits into from
Apr 1, 2021

Conversation

francofusco
Copy link
Contributor

Hello!

I want to propose this PR, which slightly changes how StaticObjects work.

Motivation and main proposal

The motivation is the following: if I want to insert a new static object, I have to inherit from StaticObject and add fields to set/get its pose. This is exactly what StaticGate was doing. However, this would likely mean that to create a new static object I should "copy-paste" static_gate.hpp/cpp and update it accordingly, e.g., by simply changing the class name. However, I think this should be discouraged since it is not very DRY.

My proposal is thus the following: let's make all static objects behave like the static gate. This means that a StaticObject now features, in addition to the id_ and prefab_id_ fields, protected variables representing the pose of the object (as well as the size). This means that now to spawn different objects one simply needs to change the prefab_id value passed to the constructor. Finally, the StaticGate class now becomes a simple shorthand for calling StaticGate's constructor with "rpg_gate" as prefab_id.

Detailed description of the changes

StaticObject

The class StaticObject is now non-abstract and can be used directly to spawn any object that does not require simulation of its dynamics. It is, more or less, a copy of what StaticGate was before this PR. The main changes are the following:

  • It now features the protected fields position_, quat_ and scale_. To access and modify them, the corresponding getters and setters were moved from StaticGate to StaticObject. These methods are kept virtual, meaning that one can change how they behave if needed.
  • The name of the getters have been updated to match the "verbose" formatting used for setters. This means that getPosition is now used instead of getPos.
  • getRotation and setRotation have been changed into setQuaternion and getQuaternion to make code clearer.
  • getID and getPrefabID now return const std::string&, i.e., by reference.

StaticGate

Since its internal mechanics is now included into StaticObject, the class is now almost a "one-liner convenience class" that allows to shorten the creation of a gate as a StaticObject:

// Any object can be spawn in this way:
StaticObject object("object_name", "asset_name");
// E.g., to spawn the gate we should use this:
StaticObject gate("gate_name", "rpg_gate");
// But (almost) the same can be done without having to remember the name of the "rpg_gate" asset: 
StaticGate another_gate("another_gate_name");

Given how simple its definition has become, I have removed static_gate.cpp and kept the full class definition inside static_gate.hpp. Let me know if you prefer to separate declaration and implementation!

Note that by making this class so minimal, a (potential) sub-issue has been also fixed: the private fields id_ and prefab_id_ were both appearing in StaticObject and StaticGate. While this is not illegal, it was most likely unintended?

UnityBridge

I simply updated its .cpp file so that it uses the new API. This simply means calling getPosition() instead of getPos() and getQuaternion() instead of getQuat().

race executable

In race.cpp (from the package flightros) I have updated the instructions that spawn the gates to show that the same gate can be inserted using either StaticObject or StaticGate. I also added few lines to demonstrate that (and how) the position of a StaticObject can be updated if needed.

types.hpp

I just saw that "rows" was misspelled in three doc-comments. I simply corrected the word where needed: no code-change was performed!

Conclusion

I hope this PR helps improving Flightmare 😄 I am of course open to discussion after your reviews 👍

In few comments, `ros` was written instead of `rows`.
- Make `StaticObject` concrete (no pure virtuals). Allow subclasses to 
still change internal mechanics by keeping seters and getters virtual.
- Make `StaticGate` nothing but a shorthand to spawn the "rpg_gate" 
asset. This also solves a shadowning issue.
- Change `getPos` into `getPosition` (so that it matches formatting of 
`setPosition`).
- Return `id_` and `prefab_id_` by const reference.
- Updated `unity_bridge.cpp` to use the new API.
- Updated `racing.cpp` (from the `flightros` pkg) to show and use the 
new API.
Make sure that sub-classes of `StaticObject` can change the pose and 
size of the object (by making the corresponding variables protected).
@slimeth slimeth requested a review from yun-long March 30, 2021 14:01
Copy link
Contributor

@yun-long yun-long left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. A minor comment.

@francofusco
Copy link
Contributor Author

Thanks @yun-long for having reviewed the PR.

Latest changes:

  • I have changed the constructor of StaticGate so that it accepts the prefab_id parameter, with a default of "rpg_gate" (as suggested)
  • Changed the parameters passed to the constructors of StaticObject and StaticGate from std::string to const std::string& (I missed it in the first commits)

Also, sorry for all the formatting-related commits, I keep forgetting to run clang before committing... 😓

Copy link
Contributor

@yun-long yun-long left a comment

Choose a reason for hiding this comment

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

lgtm

@yun-long yun-long merged commit 17778ad into uzh-rpg:master Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants