Skip to content

HYDRA-1071 : Add a flow viewport example that uses a custom GLSLFX shader for Storm #142

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.

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

Merged
merged 10 commits into from
Jul 9, 2024

Conversation

lanierd-adsk
Copy link
Collaborator

No description provided.

@lanierd-adsk lanierd-adsk self-assigned this Jul 5, 2024
@lanierd-adsk lanierd-adsk self-assigned this Jul 8, 2024
Copy link
Collaborator

@debloip-adsk debloip-adsk left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Only minor touchups and questions

Comment on lines 56 to 67
if (IS_WINDOWS)
configure_file("plugInfo_Win.json" ${CMAKE_CURRENT_BINARY_DIR}/plugInfo.json)
else
(
if (IS_LINUX)
configure_file("plugInfo_Linux.json" ${CMAKE_CURRENT_BINARY_DIR}/plugInfo.json)
endif()
if (IS_MACOSX)
configure_file("plugInfo_OSX.json" ${CMAKE_CURRENT_BINARY_DIR}/plugInfo.json)
endif()
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor : Wouldn't it be more consistent if we treated all platforms the same way? i.e. I don't see why the if (IS_WINDOWS) case is handled separately from Linux and OSX

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cannot do this or haven't found a way to do so. The name of the dynamic library is different under windows/Linx and OSX and havcing a single file like I did originally works only on Windows.

Copy link
Collaborator

@debloip-adsk debloip-adsk Jul 9, 2024

Choose a reason for hiding this comment

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

Ah sorry I meant just the CMake if cases, not the plugInfo files themselves. Just that I don't see why the Windows and OSX/Linux cases are nested differently. But it is a nitpick, though I see you've changed it

@@ -0,0 +1,5 @@
This is a pure usd plugin that is a shaders discovery plugin.
The shaders defined inshadersDef.usda will be loaded by usd/hydra and available for use in a material network in any Hydra plugin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo : missing space in "inshadersDef.usda"

Comment on lines +1 to +23
//
// Copyright 2018 Pixar
//
// Licensed under the Apache License, Version 2.0 (the "Apache License")
// with the following modification; you may not use this file except in
// compliance with the Apache License and the following modification to it:
// Section 6. Trademarks. is deleted and replaced with:
//
// 6. Trademarks. This License does not grant permission to use the trade
// names, trademarks, service marks, or product names of the Licensor
// and its affiliates, except as required to comply with Section 4(c) of
// the License and to reproduce the content of the NOTICE file.
//
// You may obtain a copy of the Apache License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the Apache License with the above modification is
// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the Apache License for the specific
// language governing permissions and limitations under the Apache License.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the Pixar license?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It comes from a Pixar's file from their examples provided with USD, This license was left also in the file from Fusion, so I left it as well.

Comment on lines +1 to +23
//
// Copyright 2018 Pixar
//
// Licensed under the Apache License, Version 2.0 (the "Apache License")
// with the following modification; you may not use this file except in
// compliance with the Apache License and the following modification to it:
// Section 6. Trademarks. is deleted and replaced with:
//
// 6. Trademarks. This License does not grant permission to use the trade
// names, trademarks, service marks, or product names of the Licensor
// and its affiliates, except as required to comply with Section 4(c) of
// the License and to reproduce the content of the NOTICE file.
//
// You may obtain a copy of the Apache License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the Apache License with the above modification is
// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the Apache License for the specific
// language governing permissions and limitations under the Apache License.
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the Pixar license?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above, it's a file originally made by Pixar.

@@ -24,8 +24,10 @@
#include <pxr/imaging/hd/materialNetworkSchema.h>
#include <pxr/imaging/hd/materialNodeSchema.h>
#include <pxr/imaging/hd/materialSchema.h>
#include <pxr/imaging/hd/perfLog.h>//For HD_TRACE_FUNCTION
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems unused

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's actually used for HD_TRACE_FUNCTION, removing it doesn't compile on different platforms.

@lilike-adsk lilike-adsk merged commit f8d12a6 into dev Jul 9, 2024
10 checks passed
@lilike-adsk lilike-adsk deleted the lanierd/HYDRA-1071 branch July 9, 2024 14:40
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.

3 participants