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

Add is_complete() to UniformInput and Implement Checks in Demos #1718

Merged
merged 6 commits into from
Dec 3, 2024

Conversation

ZzzhHe
Copy link
Contributor

@ZzzhHe ZzzhHe commented Nov 28, 2024

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:

  • After Creation by new_uniform_input(): Might miss uniforms that are added later using the update() method.
  • After Every update(): This could introduce unnecessary overhead, especially if updates occur frequently.
  • Before Rendering: Might not catch updates made inside the rendering loop.

I'm looking forward to any feedback!

Copy link
Member

@TheJJ TheJJ left a 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.

@ZzzhHe ZzzhHe force-pushed the check-uniforms-set branch from 9ccbca7 to e847fe6 Compare November 30, 2024 02:40
@ZzzhHe
Copy link
Contributor Author

ZzzhHe commented Nov 30, 2024

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.

Thanks! I've added a check_uniform_completeness() function that checks all uniforms by iterating over the Renderable objects.

Copy link
Member

@heinezen heinezen left a 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.

libopenage/renderer/demo/demo_0.cpp Outdated Show resolved Hide resolved
libopenage/renderer/demo/demo_0.cpp Outdated Show resolved Hide resolved
libopenage/renderer/demo/util.cpp Outdated Show resolved Hide resolved
libopenage/renderer/demo/util.h Outdated Show resolved Hide resolved
libopenage/renderer/util.cpp Outdated Show resolved Hide resolved
libopenage/renderer/util.h Outdated Show resolved Hide resolved
@ZzzhHe ZzzhHe force-pushed the check-uniforms-set branch from c778158 to ce5b9b6 Compare November 30, 2024 16:25
@ZzzhHe
Copy link
Contributor Author

ZzzhHe commented Nov 30, 2024

Very nice! I recommend you use clang-format on all files to solve your style issues.

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.

@ZzzhHe ZzzhHe force-pushed the check-uniforms-set branch from ce5b9b6 to 5be9a71 Compare November 30, 2024 16:29
@ZzzhHe
Copy link
Contributor Author

ZzzhHe commented Nov 30, 2024

BTW, I noticed some code in the demo files is still using ! instead of not. Should I go ahead and change that?

@heinezen
Copy link
Member

@ZzzhHe yes please :)

@ZzzhHe ZzzhHe force-pushed the check-uniforms-set branch from d774e15 to c42c9ec Compare November 30, 2024 19:36
@ZzzhHe
Copy link
Contributor Author

ZzzhHe commented Nov 30, 2024

@ZzzhHe yes please :)

done.

TheJJ
TheJJ previously requested changes Nov 30, 2024
Copy link
Member

@TheJJ TheJJ left a 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);
Copy link
Member

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})) {
Copy link
Member

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.

@heinezen
Copy link
Member

heinezen commented Nov 30, 2024

@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 ShaderUpdate). Also, the uniforms may not be initialized when the render pass is created. Uniform initialization should be deferrable for optimization purposes, so that we can create a bunch of render objects without waiting for updates from the gamestate.

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.

heinezen
heinezen previously approved these changes Dec 3, 2024
libopenage/renderer/demo/util.cpp Outdated Show resolved Hide resolved
@heinezen heinezen enabled auto-merge December 3, 2024 03:44
@heinezen heinezen dismissed TheJJ’s stale review December 3, 2024 03:45

We discussed the reasons for not doing the check inside the renderer in #1718 (comment)

@heinezen heinezen merged commit 58e6977 into SFTtech:master Dec 3, 2024
13 checks passed
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.

Optionally check if all uniform values have been set
4 participants