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

Fix demo_WheeledGeneric when Irrlicht is not available #197

Merged

Conversation

alhirzel
Copy link

@alhirzel alhirzel commented Nov 7, 2019

There was a reference to Irrlicht that was not #ifdef'd. I also #ifdefd some of the visualization configuration to better show what is not needed when visualization is unused.

@alhirzel alhirzel force-pushed the WheeledGeneric-irrlichtless-fix branch 2 times, most recently from 12f468b to 0018a83 Compare November 7, 2019 20:49
@alhirzel alhirzel changed the title Fix demo_WheeledGeneric when Irrlicht is not available WIP: Fix demo_WheeledGeneric when Irrlicht is not available Nov 7, 2019
@alhirzel alhirzel force-pushed the WheeledGeneric-irrlichtless-fix branch from 0018a83 to 489fe65 Compare November 7, 2019 20:52
@alhirzel alhirzel changed the title WIP: Fix demo_WheeledGeneric when Irrlicht is not available Fix demo_WheeledGeneric when Irrlicht is not available Nov 7, 2019
@rserban
Copy link
Member

rserban commented Nov 7, 2019

I'm not sure I understand the point of this change. This demo is not built unless the Chrono::Irrlicht module is enabled (see its CMakeLists.txt). The "generic" vehicle is used mostly as a sandbox for developing new vehicle features and general testing and so we do want this demo to always have visualization enabled.

@alhirzel
Copy link
Author

alhirzel commented Nov 7, 2019

When I saw the issue that e4410d7 fixes, I did a look-around to ensure it did not affect any other examples. In doing so, I noticed the following have the same spirit of being able to build without Irrlicht:

  • vehicle/demo_ISO2631/demo_VEH_Shock.cpp
  • vehicle/demo_M113_Band/demo_VEH_M113_Band.cpp
  • vehicle/demo_TrackedJSON/demo_VEH_TrackedJSON.cpp
  • vehicle/demo_TrackedJSON_Band/demo_VEH_TrackedJSON_Band.cpp
  • vehicle/demo_WheeledGeneric/demo_VEH_WheeledGeneric.cpp
  • vehicle/demo_WheeledJSON/demo_VEH_WheeledJSON.cpp

You raise an interesting point that these are not built when ENABLE_MODULE_IRRLICHT is unset; perhaps this behavior can be safely changed, so these demos are built whether or not Irrlicht is available. Would you mind if I addressed that in this PR as well? Alternatively, it seems like the build-time detection of Irrlicht is unmotivated, and perhaps it should be removed. (I would prefer to not do the latter, because I actually find this example very instructive.)

@rserban
Copy link
Member

rserban commented Nov 8, 2019

You're right. There are some inconsistencies here.

  • some of these demos should not have the "no run-time visualization" option (e.g., demo_VEH_WheeledGeneric and demo_VEH_TrackedJSON) because they are just a demonstration of the vehicle and really meant to be controlled through keyboard
  • others (the "Band" tracked vehicles) have it because they are too computationally expensive to run close to real time and therefore interactive driving is not the best option
  • others still have this option because they implement particular tests (e.g. demo_VEH_Shock), don't have an interactive driver, and thus could be run without visualization (for example in batch testing)

Having said that, it's great you want to help with a bit of house cleaning.

  • modify the corresponding CMakeLists.txt files to allow a particular demo to be run without Irrlicht if it already has the code for that (that is, eliminate the check for ENABLE_MODULE_IRRLICHT where appropriate). Fix the demos as needed so that they built if Irrlicht is not enabled.
  • but please do not change demos so that visualization assets are added conditional on Irrlicht being enabled! This is because visualization assets may still be used for other rendering methods (such as postprocessing with POV-Ray or the likes). Definition of visualization assets and actual run-time rendering are two different things.

@alhirzel
Copy link
Author

alhirzel commented Nov 8, 2019

Good point on the visualization assets; that change is wrong, will revert. Thank you for the guidance!

@alhirzel
Copy link
Author

@rserban I'm happy with these changes, can you take another look? I tested by building with and without MODULE_ENABLE_IRRLICHT, and found+fixed a few issues.

@rserban
Copy link
Member

rserban commented Nov 17, 2019

Looks good to me. But please squash all these commits into a single one.

@alhirzel alhirzel force-pushed the WheeledGeneric-irrlichtless-fix branch from b170289 to 587887a Compare November 17, 2019 21:29
@alhirzel
Copy link
Author

squashed

@rserban rserban merged commit 908bad9 into projectchrono:develop Nov 17, 2019
@alhirzel alhirzel deleted the WheeledGeneric-irrlichtless-fix branch November 17, 2019 21:51
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