Vapor API WIP #2856
Vapor API WIP #2856
Conversation
| if (!_glManager) _glManager = new GLManager; | ||
|
|
||
| static bool temp = false; | ||
| if (!temp) { |
There was a problem hiding this comment.
Can we use a member variable here?
There was a problem hiding this comment.
This is some temporary code so the demo works for the WIP and there is a TODO reminder to remove it 2 lines down. It will be removed later.
There was a problem hiding this comment.
Nice work, Stas. A few comments and questions:
- Presumably most of the new source file will move from apps/vapi to somewhere in 'lib'?
- Is there a reason that we need yet another logging facility? For the sake of consistency it would certainly be preferable to find a way to make use of our existing error reporting mechanism (even if some refactoring is needed).
- Given that this is work in progress would it make sense to create a feature branch rather than merging into main? I don't think there is any harm in merging this PR into main because it doesn't touch the existing code in any significant way. But as we move forward that may change. What are your thoughts, @sam, @StasJ?
Yes, it is currently acting as a command line utility which is why I placed it in apps but I will move it eventually.
I added this because I believe it is a better fit for a python API than WASP::MyBase which works better for a GUI application. I can remove it eventually but I'd prefer to leave it during development as it has features that make debugging easier.
I'm fine with doing either. We could just close the PR once the comments are addressed and have this be the feature branch. |
@StasJ, what I'm hoping to avoid is having to re-review the same code later, but at the same time I understand the need to have some of the code be a WIP that should be cleaned up later. I also don't want us to rely on anyone's memory to go back and remove temporary code, commented out code etc. Perhaps the way to proceed here is to open an issue that identifies the cleanup items, and get this PR merged. Whether it should be merged to main or to a feature branch is another topic. Thoughts on this? |
I am okay with opening an issue. We could just leave those comments unresolved and open an issue linking to this PR to resolve the unresolved comments before the final PR is accepted. |
|
Opening a separate issue to resolve unresolved comments in this PR seems both efficient, and the least likely to missing something. Let's try that. |
Done: #2869 |
|
Great. I will approve as soon as the ubuntu test failures are resolved. |
This is an initial version of the Vapor API. It provides a basis to utilize the vapor through a scripting interface. It will also provide the ability to remove the majority of the control code from the GUI. The current functionality allows for rendering session files. For demo purposes, it compiles as an executable rather than as a library. In
vapi/main.cpp, you can specify a session file to load as well as an image to output.Note: you need to enable BUILD_PYTHON in cmake to build. Also, the test driver looks for a file named 'session.vs3' and outputs a file named 'out.png'