Remove MslRenderer camera initialization #1465

nicolassavva-autodesk · 2023-08-18T23:13:48Z

Allow ShaderRenderer camera to be modified by the client in subclass renderers when using Metal.

The PR removes the Metal Renderer specific camera initialization in MslRenderer (and now handled by ShaderRenderer).

This addresses the same issue in a similar way to what was previously done for the GlslRenderer:

"Improvements to ShaderRenderer"(60b1053)

nicolassavva-autodesk · 2023-08-18T23:16:36Z

@ashwinbhat @Morteeza @jstone-lucasfilm: Please have a look when you get a chance

jstone-lucasfilm · 2023-08-19T02:24:49Z

This change looks good to me, thanks @nicolassavva-autodesk, and I'd be interested in thoughts from @Morteeza and @ashwinbhat.

Morteeza · 2023-08-21T15:14:30Z

Without setting view and world information, tests generate wrong results. I believe we need to find another way to set these:

MSL output after this change:

GLSL output:

nicolassavva-autodesk · 2023-08-21T16:01:25Z

I believe the difference comes from the following line:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXRender/ShaderRenderer.cpp#L38
we can have the Metal path call createPerspectiveMatrixZP() instead of createPerspectiveMatrix()

Morteeza · 2023-08-21T16:11:51Z

true. That probably will fix the issue. Btw, I noticed metal rendering tests were broken before. I opened a PR to fix it in case you want to try it locally.

Morteeza · 2023-08-21T16:29:09Z

Also I believe this line needs to be removed too

nicolassavva-autodesk · 2023-08-21T18:19:17Z

source/MaterialXRender/ShaderRenderer.cpp

@@ -35,7 +35,12 @@ ShaderRenderer::ShaderRenderer(unsigned int width, unsigned int height, Image::B
    float fW = fH * 1.0f;
    _camera = Camera::create();
    _camera->setViewMatrix(Camera::createViewMatrix(DEFAULT_EYE_POSITION, DEFAULT_TARGET_POSITION, DEFAULT_UP_VECTOR));
-    _camera->setProjectionMatrix(Camera::createPerspectiveMatrix(-fW, fW, -fH, fH, DEFAULT_NEAR_PLANE, DEFAULT_FAR_PLANE));
+
+    #if defined (__APPLE__)


We might also want to make this Metal specific so that it can still work with OpenGL based MacOS
@jstone-lucasfilm: what would you recommend for the cleanest way for supporting both here?
Perhaps we can move the MaterialXView specific customization to a reusable place:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXView/CMakeLists.txt#L8-L27

Should we remove the default camera in ShaderRenderer and have it setup by the caller?

I'm open to that option, @ashwinbhat, though I'm imagining that there may still be value in providing a default camera for simple use cases. As long as we continue to provide the option of overriding the default camera, it seems reasonable to make this default as suitable as possible for each platform.

@nicolassavva-autodesk What if we were to pass a perspective matrix convention boolean into the constructor for ShaderRenderer, allowing the caller to state which convention should be used for the default perspective matrix? This would allow the platform-independent ShaderRenderer class to remain agnostic of this platform detail, without requiring us to remove the default camera from ShaderRender entirely.

I introduced an enum to select between OpenGL and Metal (in lieu of a boolean)
This should suffice for now and we should eventually consider propagating the changes into a future refactor of the Camera class and the MaterialXViewer

ashwinbhat

I would recommend removing the default camera initialization ShaderRenderer so that the client can create and supply a camera that is suitable based on the underlying API implementation.
We already have setCamera API to allow this workflow.
@jstone-lucasfilm @Morteeza any thoughts on this suggestion?

ashwinbhat · 2023-08-22T19:09:37Z

source/MaterialXRender/ShaderRenderer.cpp

@@ -35,7 +35,12 @@ ShaderRenderer::ShaderRenderer(unsigned int width, unsigned int height, Image::B
    float fW = fH * 1.0f;
    _camera = Camera::create();
    _camera->setViewMatrix(Camera::createViewMatrix(DEFAULT_EYE_POSITION, DEFAULT_TARGET_POSITION, DEFAULT_UP_VECTOR));
-    _camera->setProjectionMatrix(Camera::createPerspectiveMatrix(-fW, fW, -fH, fH, DEFAULT_NEAR_PLANE, DEFAULT_FAR_PLANE));
+
+    #if defined (__APPLE__)


Should we remove the default camera in ShaderRenderer and have it setup by the caller?

nicolassavva-autodesk · 2023-08-23T15:36:20Z

Here is a sample of the render tests using the latest updates:

MSL

GLSL

Morteeza · 2023-08-23T16:37:35Z

Thanks @nicolassavva-autodesk for changes and the rendered test.

I would recommend removing the default camera initialization ShaderRenderer so that the client can create and supply a camera that is suitable based on the underlying API implementation. We already have setCamera API to allow this workflow. @jstone-lucasfilm @Morteeza any thoughts on this suggestion?

Change look good as is. But as Ashwin suggested maybe it will be more explicit to remove camera GL/Metal camera initialization, and move it to test code implementations; something like this:
patch3.diff.zip

nicolassavva-autodesk · 2023-08-23T17:02:48Z

Yes, agreed though perhaps that can wait for a future code refactor

jstone-lucasfilm

This looks like a good approach to me, thanks @nicolassavva-autodesk.

With respect to naming conventions, what would you think about the following enumerated type for the new input argument?

enum MatrixConvention
{
    MatrixOpenGL = 0,
    MatrixMetal = 1
};

This would help to clarify which convention we're specifying with this argument, and the new enumeration could potentially be moved into a lower-level library (e.g. a math library) if needed further in the future.

nicolassavva-autodesk · 2023-08-24T00:49:59Z

Yes, renaming the enums to something more explicit sounds good to me

nicolassavva-autodesk · 2023-08-24T00:56:18Z

I take it we are still keeping it as an enum class and perhaps do not need to repeat "Matrix" in the name so we get:

enum class MatrixConvention
{
    OpenGL = 0,
    Metal  = 1
};

MatrixConvention::Metal

jstone-lucasfilm · 2023-08-24T01:02:08Z

That's completely fine, and although we don't always use enum classes in MaterialX, I think it's a good direction for the future.

… camera modification).

…ialization

nicolassavva-autodesk · 2023-08-24T01:17:58Z

I just renamed the enum as discussed above and rebased the PR branch

jstone-lucasfilm

This looks good to me, thanks @nicolassavva-autodesk!

) Allow ShaderRenderer camera to be modified by the client in subclass renderers when using Metal. The PR removes the Metal Renderer specific camera initialization in MslRenderer (and now handled by ShaderRenderer).

nicolassavva-autodesk commented Aug 21, 2023

View reviewed changes

nicolassavva-autodesk force-pushed the savvan/MSLshader_render_fix branch from d385cc7 to 169d64a Compare August 21, 2023 18:22

ashwinbhat reviewed Aug 22, 2023

View reviewed changes

nicolassavva-autodesk force-pushed the savvan/MSLshader_render_fix branch 2 times, most recently from f01b0bb to c00d0b4 Compare August 23, 2023 13:51

nicolassavva-autodesk requested review from ashwinbhat and jstone-lucasfilm August 23, 2023 15:36

nicolassavva-autodesk force-pushed the savvan/MSLshader_render_fix branch from c00d0b4 to ba0c7db Compare August 23, 2023 22:20

jstone-lucasfilm changed the title ~~Remove MSLRenderer Camera initialization~~ Remove MslRenderer camera initialization Aug 24, 2023

jstone-lucasfilm reviewed Aug 24, 2023

View reviewed changes

nicolassavva-autodesk added 5 commits August 23, 2023 18:17

Remove MSLRenderer Camera initialization (enables client ShaderRender…

6f3ce20

… camera modification).

Remove MSL camera reinitialization

fafcf39

Allow Metal specific default perspective camera in ShaderRenderer

96cf6b4

Introduce enum for Metal or OpenGL ShaderRenderer default camera init…

414623c

…ialization

Rename ShaderRenderer conventions enum

d0d21fc

nicolassavva-autodesk force-pushed the savvan/MSLshader_render_fix branch from 2110b08 to d0d21fc Compare August 24, 2023 01:17

jstone-lucasfilm approved these changes Aug 24, 2023

View reviewed changes

jstone-lucasfilm merged commit 84adc65 into AcademySoftwareFoundation:main Aug 24, 2023
13 checks passed

nicolassavva-autodesk deleted the savvan/MSLshader_render_fix branch October 23, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MslRenderer camera initialization #1465

Remove MslRenderer camera initialization #1465

nicolassavva-autodesk commented Aug 18, 2023

nicolassavva-autodesk commented Aug 18, 2023

jstone-lucasfilm commented Aug 19, 2023

Morteeza commented Aug 21, 2023 •

edited

Loading

nicolassavva-autodesk commented Aug 21, 2023

Morteeza commented Aug 21, 2023

Morteeza commented Aug 21, 2023

nicolassavva-autodesk Aug 21, 2023

ashwinbhat Aug 22, 2023

jstone-lucasfilm Aug 23, 2023

jstone-lucasfilm Aug 23, 2023

nicolassavva-autodesk Aug 23, 2023

ashwinbhat left a comment •

edited

Loading

ashwinbhat Aug 22, 2023

nicolassavva-autodesk commented Aug 23, 2023

Morteeza commented Aug 23, 2023

nicolassavva-autodesk commented Aug 23, 2023

jstone-lucasfilm left a comment

nicolassavva-autodesk commented Aug 24, 2023

nicolassavva-autodesk commented Aug 24, 2023 •

edited

Loading

jstone-lucasfilm commented Aug 24, 2023

nicolassavva-autodesk commented Aug 24, 2023

jstone-lucasfilm left a comment

Remove MslRenderer camera initialization #1465

Remove MslRenderer camera initialization #1465

Conversation

nicolassavva-autodesk commented Aug 18, 2023

nicolassavva-autodesk commented Aug 18, 2023

jstone-lucasfilm commented Aug 19, 2023

Morteeza commented Aug 21, 2023 • edited Loading

nicolassavva-autodesk commented Aug 21, 2023

Morteeza commented Aug 21, 2023

Morteeza commented Aug 21, 2023

nicolassavva-autodesk Aug 21, 2023

Choose a reason for hiding this comment

ashwinbhat Aug 22, 2023

Choose a reason for hiding this comment

jstone-lucasfilm Aug 23, 2023

Choose a reason for hiding this comment

jstone-lucasfilm Aug 23, 2023

Choose a reason for hiding this comment

nicolassavva-autodesk Aug 23, 2023

Choose a reason for hiding this comment

ashwinbhat left a comment • edited Loading

Choose a reason for hiding this comment

ashwinbhat Aug 22, 2023

Choose a reason for hiding this comment

nicolassavva-autodesk commented Aug 23, 2023

Morteeza commented Aug 23, 2023

nicolassavva-autodesk commented Aug 23, 2023

jstone-lucasfilm left a comment

Choose a reason for hiding this comment

nicolassavva-autodesk commented Aug 24, 2023

nicolassavva-autodesk commented Aug 24, 2023 • edited Loading

jstone-lucasfilm commented Aug 24, 2023

nicolassavva-autodesk commented Aug 24, 2023

jstone-lucasfilm left a comment

Choose a reason for hiding this comment

Morteeza commented Aug 21, 2023 •

edited

Loading

ashwinbhat left a comment •

edited

Loading

nicolassavva-autodesk commented Aug 24, 2023 •

edited

Loading