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 basic materials scene renderer proj #428

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add basic materials scene renderer proj #428

wants to merge 1 commit into from

Conversation

anishmgoyal
Copy link
Collaborator

This demonstrates the scene renderer for basic materials
@anishmgoyal @chaoticbob @apazylbe
Add basic materials scene renderer proj
904c845 
This demonstrates the scene renderer for basic materials

Co-authored-by: Hai Nguyen <chaoticbob@users.noreply.github.com>
Co-authored-by: Aliya Pazylbekova <apazylbe@users.noreply.github.com>
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 reviewed May 8, 2024
View reviewed changes
projects/gltf_basic_materials/CMakeLists.txt
@@ -0,0 +1,30 @@
# Copyright 2023 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

2024
projects/gltf_basic_materials/CMakeLists.txt
# limitations under the License.
cmake_minimum_required(VERSION 3.0 FATAL_ERROR)

project(31_gltf_basic_materials)
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 we can remove 31_
projects/gltf_basic_materials/GltfBasicMaterials.cpp
@@ -0,0 +1,339 @@
// Copyright 2023 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

2024
projects/gltf_basic_materials/main.cpp
@@ -0,0 +1,17 @@
// Copyright 2023 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

2024
projects/gltf_basic_materials/GltfBasicMaterials.cpp
settings.appName = "gltf_basic_materials";
settings.enableImGui = true;
settings.grfx.api = kApi;
settings.grfx.enableDebug = true;
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 we should just remove this line and rely on the default behaviour
projects/gltf_basic_materials/GltfBasicMaterials.cpp
//
PPX_CHECKED_CALL(scene::GltfLoader::Create(
GetAssetPath("scene_renderer/scenes/tests/gltf_test_basic_materials.glb"),
// GetAssetPath("scene_renderer/scenes/tests/gltf_test_materials.gltf"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably remove this line
projects/gltf_basic_materials/GltfBasicMaterials.cpp
PPX_CHECKED_CALL(grfx_util::CreateIBLTexturesFromFile(
GetDevice()->GetGraphicsQueue(),
GetAssetPath("poly_haven/ibl/old_depot_4k.ibl"),
// GetAssetPath("poly_haven/ibl/abandoned_workshop_02_4k.ibl"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably remove this line
Copy link
Member

Choose a reason for hiding this comment

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

And remove the new assets since they seem to be unused.
footballhead
footballhead reviewed May 8, 2024
View reviewed changes
projects/gltf_basic_materials/GltfBasicMaterials.cpp
PPX_CHECKED_CALL(frame.imageAcquiredFence->WaitAndReset());

// Wait for and reset render complete fence
PPX_CHECKED_CALL(frame.renderCompleteFence->WaitAndReset());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait on renderCompleteFence before AcquireNextImage, see #445
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is the wait for imageAcquiredFence() required here?
Could we do:

  • renderCompleteFence.WaitAndReset()
  • acquireNextImage(imageAcquiredSemaphore)
  • graphics.Submit(wait=imageAcquiredSemaphore, signal=renderCompleteSemaphore + fence)
  • present(wait=renderCompleteSemaphore)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Keenuts There was a thread on this in our chat last November, where someone mentioned acquireNextImage return with invalid index, I am not sure how though.
footballhead
footballhead reviewed May 8, 2024
View reviewed changes
projects/gltf_basic_materials/GltfBasicMaterials.h
: public ppx::Application
{
public:
virtual void Config(ppx::ApplicationSettings& settings) override;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: virtual is implied by override and can be removed
Keenuts
Keenuts reviewed May 13, 2024
View reviewed changes
Copy link
Member

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
projects/gltf_basic_materials/GltfBasicMaterials.cpp
Comment on lines +33 to +34
settings.window.width = 1920 * 1;
settings.window.height = 1080 * 1;
Copy link
Member

Choose a reason for hiding this comment

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

why * 1? Might be removed. (Might also keep the default value?)
projects/gltf_basic_materials/GltfBasicMaterials.cpp
PPX_CHECKED_CALL(grfx_util::CreateIBLTexturesFromFile(
GetDevice()->GetGraphicsQueue(),
GetAssetPath("poly_haven/ibl/old_depot_4k.ibl"),
// GetAssetPath("poly_haven/ibl/abandoned_workshop_02_4k.ibl"),
Copy link
Member

Choose a reason for hiding this comment

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

And remove the new assets since they seem to be unused.
projects/gltf_basic_materials/GltfBasicMaterials.cpp
PPX_CHECKED_CALL(frame.imageAcquiredFence->WaitAndReset());

// Wait for and reset render complete fence
PPX_CHECKED_CALL(frame.renderCompleteFence->WaitAndReset());
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is the wait for imageAcquiredFence() required here?
Could we do:

  • renderCompleteFence.WaitAndReset()
  • acquireNextImage(imageAcquiredSemaphore)
  • graphics.Submit(wait=imageAcquiredSemaphore, signal=renderCompleteSemaphore + fence)
  • present(wait=renderCompleteSemaphore)
@footballhead footballhead mentioned this pull request Jun 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants
@anishmgoyal @footballhead @Keenuts @apazylbe