Skip to content

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

Merged
heinezen merged 6 commits into
SFTtech:masterfrom
ZzzhHe:check-uniforms-set
Dec 3, 2024
Merged

Add is_complete() to UniformInput and Implement Checks in Demos #1718
heinezen merged 6 commits into
SFTtech:masterfrom
ZzzhHe:check-uniforms-set

Conversation

@ZzzhHe

@ZzzhHe ZzzhHe commented Nov 28, 2024

Copy link
Copy Markdown
Contributor

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!

Comment thread libopenage/renderer/opengl/uniform_input.cpp

@TheJJ TheJJ left a comment

Copy link
Copy Markdown
Member

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

ZzzhHe commented Nov 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@heinezen heinezen left a comment

Copy link
Copy Markdown
Member

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.

Comment thread libopenage/renderer/demo/demo_0.cpp Outdated
Comment thread libopenage/renderer/demo/demo_0.cpp Outdated
Comment thread libopenage/renderer/demo/util.cpp Outdated
Comment thread libopenage/renderer/demo/util.h
Comment thread libopenage/renderer/util.cpp
Comment thread libopenage/renderer/util.h
@ZzzhHe
ZzzhHe force-pushed the check-uniforms-set branch from c778158 to ce5b9b6 Compare November 30, 2024 16:25
@ZzzhHe

ZzzhHe commented Nov 30, 2024

Copy link
Copy Markdown
Contributor Author

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

ZzzhHe commented Nov 30, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

@ZzzhHe yes please :)

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

ZzzhHe commented Nov 30, 2024

Copy link
Copy Markdown
Contributor Author

@ZzzhHe yes please :)

done.

TheJJ
TheJJ previously requested changes Nov 30, 2024

@TheJJ TheJJ left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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.

{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
Copy Markdown
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

heinezen commented Nov 30, 2024

Copy link
Copy Markdown
Member

@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
Comment thread libopenage/renderer/demo/util.cpp Outdated
@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
@ZzzhHe
ZzzhHe deleted the check-uniforms-set branch January 19, 2025 20:11
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