brianzinn / react-babylonjs

React for Babylon 3D engine
https://brianzinn.github.io/react-babylonjs/
824 stars 105 forks source link

is missing clearCoat property in <pbrMaterial />? #114

Closed alexhoma closed 3 years ago

alexhoma commented 3 years ago

Hi, it seems that there is no clearCoat property and its viariants isEnabled, intensity, roughness, etc. in <pbrMaterial />. Or maybe I don't understand which is the signature for this component. Am I right?

If those properties are really missing I can help to add it. :smile:

brianzinn commented 3 years ago

good question. the clearCoat is missing because it's a readonly property, but there is no good way right now to access the sub-properties. One thing I could do is something like the kebab accessors that I added for Vector3, which for position are position-x, position-y and position-z. You end up with something like: clearCoat-intensity, etc, but there would be so many like that from other readonly properties that it would require a bunch of optimisations (would be poor performance currently), so then I'm wondering if something could be done like how textures are done declaratively and we could add a new property called assignFrom (opposite functionality of current property assignTo).

The syntax then becomes this:

<pbrMaterial ...>
  <pbrClearCoatConfiguration assignFrom='clearCoat' intensity={0.7} ... />
</pbrMaterial>

How does that look?

alexhoma commented 3 years ago

Hi brian! sorry for the late answer. Well I didn't know that clearCoat poperty was readonly (I'm pretty new at Babylon). What do you mean about this? I thought that I was able to set something like this (or at least, the library allows me do it):

const material = new PBRMaterial('my-material', scene);
material.clearCoat.intensity = 0.5;
material.clearCoat.roughness = 1;

The declarative way looks good to me, but I don't know if this is a standard on how to set this type of props in react-babylon or it is just a temporary workaround that needs to be thought better?

brianzinn commented 3 years ago

Sorry, I meant that the property itself was readonly (https://github.com/BabylonJS/Babylon.js/blob/master/src/Materials/PBR/pbrBaseMaterial.ts#L817). The sub-properties are all updateable, so that's why I made the declarative proposal to access the configuration, so you could set intensity as in the example. If that declarative examples looks good then I'm able to start coding. It's a bit of work as I'll need to dig into the code generation and add assignFrom. I think the react-reconciler first creates the child and then the parent, so it will be a deferred creation not handled directly by reconciler. I think it would be a good addition for sure - do you have an example playground we can go off that showcases it is working as expected?

brianzinn commented 3 years ago

hi @alexhoma I've added assignFrom and an example for changing clearCoat on a PBR material (not in NPM yet). image Clicking the "Switch Roughness" button - you can see changes also in Scene Explorer Materials Inspector image

There's no built-in support for the ClearCoatConfiguration. I'm not really sure if those Configurations should all be generated. I haven't actually looked at how that will affect bundle size, but I can add the ones for materials - I think it adds a really great way to declaratively work with loaded models and is a good addition - I always wanted to be able to declare this way, but never got around to it. So, I'll look at adding the configurations for PBR to start. There is a sample demo if you wanted to fork the project and try, but I will try to get time this week to generate the PBR configurations otherwise. Cheers https://github.com/brianzinn/react-babylonjs/blob/master/stories/babylonjs/Textures/dynamic-configuration.stories.js

brianzinn commented 3 years ago

Have ran it through the code generator with some changes. These are all the read-only properties of supported classes that have classes as properties - turns out this was limited only to materials:

 >> adding StandardMaterial.prePassConfiguration (read-only) property type 'PrePassConfiguration'
 >> adding StandardMaterial.detailMap (read-only) property type 'DetailMapConfiguration'
 >> adding PBRBaseMaterial.clearCoat (read-only) property type 'PBRClearCoatConfiguration'
 >> adding PBRBaseMaterial.anisotropy (read-only) property type 'PBRAnisotropicConfiguration'
 >> adding PBRBaseMaterial.brdf (read-only) property type 'PBRBRDFConfiguration'
 >> adding PBRBaseMaterial.sheen (read-only) property type 'PBRSheenConfiguration'
 >> adding PBRBaseMaterial.subSurface (read-only) property type 'PBRSubSurfaceConfiguration'
 >> adding PBRBaseMaterial.prePassConfiguration (read-only) property type 'PrePassConfiguration'
 >> adding PBRBaseMaterial.detailMap (read-only) property type 'DetailMapConfiguration'

Note that PrePassConfiguration doesn't have any updateable props. The changes with assignFrom have tipped the scale, so I'm going to need to write some unit tests on the renderer itself before publishing these changes. As a TDD developer this library was always an anomaly - just keeps on growing beyond what I ever expected 😃

brianzinn commented 3 years ago

I just added some very basic tests - I'll try to write a few more, but will get a new NPM out next week either way 🚀

brianzinn commented 3 years ago

Please re-open if it's not solved in 3.0.6+.