-
Notifications
You must be signed in to change notification settings - Fork 452
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
Add support for custom JSON constructors for vehicle sub-systems #490
base: main
Are you sure you want to change the base?
Conversation
First of all, thanks for contributing this PR, @edvinsternvik. I've made a similar change to my local version of Chrono that allows me to do pretty much the same thing you are proposing. I have a few questions about your implementation:
Looking forward to your thoughts as well as feedback from others in the project. |
Hello, thanks for your feedback. Yes, the example should probably be something more like:
Good suggestion, I can't think of a use case that wouldn't work with a map. I just decided to just follow the method that was already used in the code
but using a map might be a cleaner approach. Yes a global registry would reduce the number of changes required in the code. I decided not to do it that way to avoid unnecessary global state as I generally think they make the code harder to reason about. But in this case the constructors are only going to be used in a handful of places so a global registry might be better than having to pass the constructors around everywhere. I welcome any opinions on point 2 and 3, so that I can modify the code based on what we decide. |
Edvin, thank you for the contribution. |
Currently vehicles can be constructed from JSON files, but only using the predefined templates. This pull request adds the possibility to supply custom constructors when creating a vehicle from a JSON file.
This can be used like this:
(main.cpp)
(ExampleDriveline.json)