-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Added OpenPBR material and option to load using glTF #16773
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
base: master
Are you sure you want to change the base?
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16773/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/16773/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/16773/merge#BCU1XR#0 |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU |
* | ||
* This offers the main features of a standard PBR material. | ||
* For more information, please refer to the documentation : | ||
* https://doc.babylonjs.com/features/featuresDeepDive/materials/using/introToPBR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to change that headline and mention that PBR2 is an optimized version of PBR that remains for backward compat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also is PBR2Material better for a name? All the materials should end with "Material"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to PBR2Material
, as you suggest but I'm viewing that as a WIP name.
What we're interested in building is full OpenPBR support so it's been suggested here that we should just name it OpenPBRMaterial
.
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
I've been playing with mixins to try to remove a lot of duplicate code from the materials. I figured, if we can do this, it'll make my life easier when working on PBR2. I started with using mixins for some defines like UV1, UV2, etc. and the image-processing defines. Then, I tried actually making the imageProcessing logic into a mixin. This removes a whole bunch of duplicated stuff. |
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
1 similar comment
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
import type { ColorCurves } from "./colorCurves"; | ||
|
||
// Explicit re-export of types to help TypeScript resolve them in declaration files | ||
export type { Observer } from "../Misc/observable"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting errors in the npm run build:es6
involving ../..
import paths in this mixin.
An AI tool came up with the solution of adding these explicit exports. I don't really understand it but it works. It should be checked by someone who understands issues like this.
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
I've noticed another issue with my changes that I don't understand. When running a Playground in Typescript mode, I get an error about not being able to find the BABYLON namespace. Any ideas what I might have done to cause that? |
fe87eca
to
3a5b609
Compare
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
No description provided.