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 pipeline args util class #426

Merged
merged 2 commits into from
Jun 17, 2024
Merged

Add pipeline args util class #426

merged 2 commits into from
Jun 17, 2024

Conversation

anishmgoyal
Copy link
Collaborator

@anishmgoyal anishmgoyal commented Feb 13, 2024 •
edited
Loading

Utility for creating graphics pipelines. This class creates descriptors for shader inputs (standard material, unlit material, error material, debug material). It also creates samplers, etc, to simplify the creation of graphics pipelines.

footballhead added a commit to footballhead/bigwheels that referenced this pull request Apr 15, 2024
@footballhead
Update mViewProjectionMatrix after modiying mViewMatrix
3e29e6d 
I added an ArcballCamera to google#428 and noticed that it wasn't working as
expected.

ArcballCamera wasn't updating mViewProjectionMatrix after modifying
mViewMatrix. This was causing the ArcballCamera to not work correctly
when combined with the classes introduced in google#426.
@footballhead footballhead mentioned this pull request Apr 15, 2024
Merged
footballhead added a commit to footballhead/bigwheels that referenced this pull request Apr 15, 2024
@footballhead
Update mViewProjectionMatrix after modiying mViewMatrix
ea5c34a 
I added an ArcballCamera to google#428 and noticed that it wasn't working as
expected.

ArcballCamera wasn't updating mViewProjectionMatrix after modifying
mViewMatrix. This was causing the ArcballCamera to not work correctly
when combined with the classes introduced in google#426.
apazylbe pushed a commit to footballhead/bigwheels that referenced this pull request Apr 17, 2024
@footballhead @apazylbe
Update mViewProjectionMatrix after modiying mViewMatrix
859f682 
I added an ArcballCamera to google#428 and noticed that it wasn't working as
expected.

ArcballCamera wasn't updating mViewProjectionMatrix after modifying
mViewMatrix. This was causing the ArcballCamera to not work correctly
when combined with the classes introduced in google#426.
Keenuts pushed a commit that referenced this pull request Apr 17, 2024
@footballhead
Update mViewProjectionMatrix in ArcballCamera::UpdateCamera (#458)
8b979f7 
I added an ArcballCamera to #428 and noticed that it wasn't working as
expected.

ArcballCamera wasn't updating `mViewProjectionMatrix` after modifying
`mViewMatrix`. This was causing the ArcballCamera to not work correctly
when combined with the classes introduced in #426.
apazylbe
apazylbe approved these changes May 8, 2024
View reviewed changes
include/ppx/scene/scene_pipeline_args.h Outdated Show resolved Hide resolved
include/ppx/scene/scene_pipeline_args.h Outdated Show resolved Hide resolved
include/ppx/scene/scene_pipeline_args.h Outdated Show resolved Hide resolved
include/ppx/scene/scene_pipeline_args.h Show resolved Hide resolved
src/ppx/scene/scene_pipeline_args.cpp Outdated Show resolved Hide resolved
@apazylbe apazylbe requested review from Keenuts and removed request for chaoticbob May 8, 2024 15:14
@Keenuts
Copy link
Member

Keenuts commented May 13, 2024

I think this should be tested in some way before we merge it.
Would it be possible to use this in one of the samples (or copy a sample and convert is to use this) so we can at least have some "coverage"?

@apazylbe apazylbe force-pushed the addPipelineArgsUtilClass branch from a9a949b to 5e74dbd Compare June 7, 2024 18:38
@apazylbe
Copy link
Collaborator

apazylbe commented Jun 7, 2024

I think this should be tested in some way before we merge it. Would it be possible to use this in one of the samples (or copy a sample and convert is to use this) so we can at least have some "coverage"?

@footballhead Does your sample use the changes in this PR?

@footballhead
Copy link
Collaborator

I think this should be tested in some way before we merge it. Would it be possible to use this in one of the samples (or copy a sample and convert is to use this) so we can at least have some "coverage"?

@footballhead Does your sample use the changes in this PR?

Yes. It's also used in #428 so that might be worth reviewing and checking in.

@apazylbe
Copy link
Collaborator

Any objections to merging this?

anishmgoyal and others added 2 commits June 17, 2024 09:58
@anishmgoyal @chaoticbob @apazylbe
Add pipeline args util class
65a47a8 
Co-authored-by: Hai Nguyen <chaoticbob@users.noreply.github.com>
Co-authored-by: Aliya Pazylbekova <apazylbe@users.noreply.github.com>
@apazylbe
Update copyright year and comments
fe2b091
@apazylbe apazylbe force-pushed the addPipelineArgsUtilClass branch from 489397d to fe2b091 Compare June 17, 2024 13:58
@apazylbe apazylbe merged commit baf1e4a into google:main Jun 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apazylbe apazylbe apazylbe approved these changes

@Keenuts Keenuts Keenuts left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anishmgoyal @Keenuts @apazylbe @footballhead