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.

Sign up for GitHub

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

Jump to bottom

Add push constants #7817

Merged
merged 8 commits into from May 9, 2024
Merged

Add push constants #7817

merged 8 commits into from May 9, 2024

Conversation

poweifeng
Copy link
Contributor

  • Push constants is a small set of bytes that can be recorded directly on the command buffer.
  • Introduce its use for internal purposes.
  • Implemented it for the vulkan backend.

@poweifeng
Add push constants
2f2e0a6 
 - Push constants is a small set of bytes that can be recorded
   directly on the command buffer.
 - Introduce its use for internal purposes.
 - Implemented it for the vulkan backend.
@poweifeng poweifeng added the internal Issue/PR does not affect clients label May 1, 2024
pixelflinger
pixelflinger reviewed May 2, 2024
View reviewed changes
filament/backend/include/private/backend/DriverAPI.inc Outdated Show resolved Hide resolved
filament/backend/include/backend/Program.h Outdated Show resolved Hide resolved
filament/backend/include/backend/DriverEnums.h Outdated Show resolved Hide resolved
filament/backend/include/backend/DriverEnums.h Outdated Show resolved Hide resolved
filament/backend/include/backend/Program.h Outdated Show resolved Hide resolved
@poweifeng poweifeng marked this pull request as ready for review May 2, 2024 23:00
pixelflinger
pixelflinger requested changes May 3, 2024
View reviewed changes
Copy link
Collaborator

@pixelflinger pixelflinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • use std::move()/std::swap() when setting the constants in Program
  • use CString instead of char*
  • remove conceptually private stuff from EngineEnums.h

filament/backend/include/backend/Program.h Outdated Show resolved Hide resolved
filament/backend/include/private/backend/DriverAPI.inc Show resolved Hide resolved
filament/backend/src/Program.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLDriver.cpp Show resolved Hide resolved
filament/backend/src/vulkan/VulkanHandles.cpp Outdated Show resolved Hide resolved
libs/filabridge/include/private/filament/EngineEnums.h Outdated Show resolved Hide resolved
@poweifeng poweifeng force-pushed the pf/push-constant branch 2 times, most recently from dc9cb6f to 3d1880a Compare May 7, 2024 05:11
pixelflinger
pixelflinger requested changes May 7, 2024
View reviewed changes
filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.h Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.h Outdated Show resolved Hide resolved
libs/filabridge/include/private/filament/EngineEnums.h Outdated Show resolved Hide resolved
pixelflinger
pixelflinger requested changes May 8, 2024
View reviewed changes
filament/backend/src/opengl/OpenGLDriver.cpp
Comment on lines +3836 to +3841

if (!mCurrentPushConstants) {
mCurrentPushConstants = new (std::nothrow) PushConstantBundle{p->getPushConstants()};
} else {
(*mCurrentPushConstants) = p->getPushConstants();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use a value instead of a pointer since it will always be allocated, this will avoid a if here the first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to pull in OpenGLProgram.h in OpenGLDriver.h?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that should be okay.

filament/backend/src/opengl/OpenGLProgram.cpp Outdated Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.h Outdated
@@ -38,6 +38,24 @@ namespace filament::backend {

class OpenGLDriver;

struct PushConstantBundle {
uint8_t fragmentStageOffset = 0;
utils::FixedCapacityVector<std::pair<GLint, ConstantType>> const* constants = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can even make this a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used two slices instead.

filament/backend/src/opengl/OpenGLProgram.h Show resolved Hide resolved
filament/backend/src/opengl/OpenGLProgram.h Outdated Show resolved Hide resolved
@poweifeng poweifeng merged commit 6f2c45c into main May 9, 2024
11 checks passed
@poweifeng poweifeng deleted the pf/push-constant branch May 9, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bejado bejado bejado left review comments

@pixelflinger pixelflinger pixelflinger approved these changes

Assignees
No one assigned
Labels
internal Issue/PR does not affect clients
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@poweifeng @pixelflinger @bejado