A complete overhaul of vehicle and world generation through jinja as well as json. #651
Conversation
There was a problem hiding this comment.
The changes look like it is trying to solve a lot more problems than just removing static world files. Please clarify what problems the changes are trying to solve for clarity.
About static world files - I don't see much benefit from making the worlds need to be dynamic, since worlds are "static". Therefore, IMHO the toolchain around generating the world file seem like unnecessary complexity being added. For example, I cannot modify the world simply by editing the world file anymore, I have to understand how the generation works.
As we discussed on line, if this is for the workaround with the mavlink_interface not being able to proper configure the serial I would prefer we fix this properly and leave the worlds static.
| @@ -0,0 +1,169 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
This file looks like a duplicate of jinja_gen.py. Why was this file added?
There was a problem hiding this comment.
I'm leaving jinja_gen.py for now but it will be deleted/deprecated in the future, this allows for generation of models even from json source as well as checking for model options etc.
There was a problem hiding this comment.
@bperseghetti let's make this more consistent then. If we are replacing something, let's remove the part being replaced and not leave there as a remaining.
There was a problem hiding this comment.
I plan to give a one month phase out to cover any potential unforeseen issues and soft-add any extra desired features before eliminating the other pipeline.
There was a problem hiding this comment.
@bperseghetti I think these changes should be atomic given that they are part of CI
There was a problem hiding this comment.
So would you like me to then add this one instead into CMake to use for default builds?
There was a problem hiding this comment.
Yes, or make this jinja_gen.py? End of the day, it is functionally it is completely redundant right?
There was a problem hiding this comment.
Well, the reason why I left jinja_gen.py in place for now was this current implementation needs to be changed to allow for default generation from CMake or CMake needs to be modified to use it properly. So they are not actually "redundant". But I'll change CMake I guess if we want to eliminate the other script completely.
There was a problem hiding this comment.
If it is not redundant, we can leave it - but I didn't quite understand what you mean by not completely redundant. What is different? As far as I can see, they are both used to generate models from the same template?
| @@ -0,0 +1,169 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
@bperseghetti let's make this more consistent then. If we are replacing something, let's remove the part being replaced and not leave there as a remaining.
|
This will need docs. |
|
Hey @hamishwillee this PR has lots of documentation any ideas where we this could live on our docs? |
bperseghetti
requested review from
Jaeyoung-Lim
and removed request for
jkflying
|
Fixes #642 |
scripts/jinja_cmake_gen.py
Outdated
|
|
||
| with open(filename_out, 'w') as f_out: | ||
| print(('{:s} -> {:s}'.format(args.filename, filename_out))) | ||
| f_out.write(result) |
There was a problem hiding this comment.
Maybe I am just not understanding this correctly, but why is this needed if we have jinja_model_gen.py?
There was a problem hiding this comment.
This is to decouple potential errors in the build from new features in the "full options" generator. This is intentionally meant to be extremely simple and used exclusively to build all the defaults with cmake, the other is for user optional generation. We can remove this in the future if desired but for now I think it is important to keep.
There was a problem hiding this comment.
@bperseghetti Okay, but my comment on having two redundant scripts was precisely coming from having two ways of doing the same functionality and one being untested. I would prefer if the full blown script is still part of the pipeline. Otherwise this will just rot unless somebody is actively using it
There was a problem hiding this comment.
Not sure if I'm completely following what you are saying. Can you elaborate more?
There was a problem hiding this comment.
As far as I understand, jinja_cmake_gen.py is completely redundant functionally with jinja_model_gen.py. However, while jinja_cmake_gen.py is tested in our CI pipeline(we run SITL tests with it) jinja_model_gen.py is not being tested. Therefore if we are to make changes in the future to jinja_cmake_gen.py we will need to manually copy the changes into jinja_model_gen.py. Therefore I want to have a single script so that it has lower maintenance overhead. OR we need to add tests that use jinja_model_gen.py as a separate integration test
There was a problem hiding this comment.
Keeping separate how I originally intended, there should be no issue with doing that.
There was a problem hiding this comment.
I am still not clear on what is blocking us from using the full blown script as the single script to do everything. For example when I want to improve the script, which one should I update? do I need to keep track of both?
There was a problem hiding this comment.
You should never need to worry about keeping jinja_gen.py up to date as any extra changes/editions can be handled with "defaults" in the individual jinja models if they are not explicitly set. And since that is how the default models are created you also do not need to worry about circle CI for jinja_model_gen.py or jinja_world_gen.py as they are "optional". I also re-added back in my Firmware PR the gazebo_sitl_multiple_run.sh so people wanting to use that still can.
There was a problem hiding this comment.
I would find it hard to see any issue with having both as it provides a cleaner "user experience" by not breaking any current pipelines, still gets tested in CI, and the other generators can be used or not used if desired.
There was a problem hiding this comment.
I don't think this issue has been resolved. So how can we make sure the generation scripts are being maintained?
…well as json. Adds features to enable wind, dynamically set sun, clouds, models for hitl, and much more.
bperseghetti
force-pushed
the
pr-hitl-capable-jinja
branch
from
b5e7100
to
aa6003f
Compare
| @@ -25,3 +25,5 @@ models/rover/rover.sdf | |||
| models/tiltrotor/tiltrotor.sdf | |||
| models/boat/boat.sdf | |||
| models/typhoon_h480/typhoon_h480.sdf | |||
| models/temp* | |||
| worlds/temp* | |||
There was a problem hiding this comment.
I can't find my previous review anymore, what is stopping us from moving this to /tmp?
|
|
||
| <author> | ||
| <name>Benjamin Perseghetti</name> | ||
| <email>[email protected]</email> |
There was a problem hiding this comment.
We have some models that are under the Apache license. Wouldn't this be problematic if we relabel the model config?
| <ambient>0.95 0.95 0.95 1</ambient> | ||
| <background>0.3 0.3 0.3 1</background> | ||
| <shadows>true</shadows> | ||
| </scene> |
There was a problem hiding this comment.
I do appreciate you fixing the lighting in gazebo, but I think this would have been better if it belonged to a separate PR
|
What is missing here to move this forward? |
|
@LorenzMeier This PR includes various changes that are orthogonal and I am slightly uncomfortable with some of the changes. Therefore I will try to break this down into multiple PRs and get some of the features in. |
This PR allows for full control over modifying models and world files on launch through jinja and optionally json. After more adoption the static world files can be completely removed. This works with PR 16142 from PX4-Autopilot.