-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 is_complete() to UniformInput and Implement Checks in Demos #1718
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the duplication in each demo/usage to check for the errors should be optimized away, it would be better to check at a place all of them invoke anyway.
9ccbca7
to
e847fe6
Compare
Thanks! I've added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I recommend you use clang-format
on all files to solve your style issues.
c778158
to
ce5b9b6
Compare
Thanks! I’ve formatted the files with clang-format. Also, I used a rebase to remove the two files that didn’t need changes, so I had to force-push. |
ce5b9b6
to
5be9a71
Compare
BTW, I noticed some code in the demo files is still using |
@ZzzhHe yes please :) |
d774e15
to
c42c9ec
Compare
done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think there's a better way to place the completeness check - in the renderer when a render pass is created.
* @param renderables The list of renderable objects to check. | ||
* @return true if all uniforms have been set, false otherwise. | ||
*/ | ||
bool check_uniform_completeness(const std::vector<Renderable> &renderables); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means we need to copy the renderables into the vector, which we don't need :)
you can convert the type to const std::vector<Renderable &> &renderables
.
if this is known at compiletime, you can do it even better by using a variadic template.
on another note: this is useful not only for the demos, but for the renderer in general. so it should be put outside the demo directory.
@@ -487,6 +489,10 @@ void RenderManagerDemo6::create_render_passes() { | |||
this->display_pass = renderer->add_render_pass( | |||
{display_obj_3d, display_obj_2d, display_obj_frame}, | |||
renderer->get_display_target()); | |||
|
|||
if (not check_uniform_completeness({display_obj_3d, display_obj_2d, display_obj_frame})) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as you see above, the render pass stores the exact same renderables, so the render pass can get the feature to check if all its uniform inputs are complete.
when the render pass is added to the renderer, the check can be invoked there directly, i think.
@TheJJ It should not be done in the renderer itself because we might get a bunch problems. We would no longer be able to make partial uniform updates for shaders (e.g. with I actually just wanted this function for debugging because uninitialized uniform values are really hard to detect :D But they are not necessary for the general workflow I think. |
1c8aee0
to
5ed0e5e
Compare
5ed0e5e
to
89719a9
Compare
We discussed the reasons for not doing the check inside the renderer in #1718 (comment)
Hi, This is my first PR! Issue resolve #1681
This PR introduces a virtual is_complete() method to UniformInput, with an override in GlUniformInput to validate that all required uniform values are set.
I'm currently a bit unsure about the optimal place to call is_complete() in the demos. Here are the options I'm considering:
new_uniform_input()
: Might miss uniforms that are added later using the update() method.update()
: This could introduce unnecessary overhead, especially if updates occur frequently.I'm looking forward to any feedback!