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.

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

Make sure "babylonTransparencyMode" attribute is correctly initialised #897

Merged
merged 15 commits into from
Dec 4, 2020
Merged

Make sure "babylonTransparencyMode" attribute is correctly initialised #897

merged 15 commits into from
Dec 4, 2020

Conversation

dkyteasSG
Copy link
Contributor

@dkyteasSG dkyteasSG commented Nov 20, 2020

fix #865

@pandaGaume
Copy link
Contributor

@dkyteasSG thanks for this work. What is the context/use case of this PR, which is the solved issue?

@dkyteasSG
Copy link
Contributor Author

Issue: Transparent material is exported with alpha=1.0.

To reproduce this issue you need to export the model more than once.
The first time, the material will be exported correctly because the babylonTransparencyMode attribute is not used.
See BabylonExporter.Material::ExportMaterial::isTransparencyModeFromBabylonMaterialNode == false (line 137).

Any subsequent exports will use the attribute to determine transparency but it will have the wrong value because it is being initialised incorrectly, so the material will be exported as Opaque.

I also noticed that Babylon modifies the maya scene by adding several babylonStandardMaterialNode nodes (the type of node that has the babylonTransparencyMode attribute) which remain there after the end of export.
Is this intentional?

@pandaGaume
Copy link
Contributor

Ok so this is related to #865.
Regarding the nodes, i'm not aware of it. We are not supposed to modify the scene structure. Please raise an issue if you think these nodes must be cleared at the end of the export.
G.

@pandaGaume
Copy link
Contributor

pandaGaume commented Nov 21, 2020

adding @Drigax
Regarding the code, il will be more confortable to not duplicate the lines and going with something like

...
if (babylonAttributesDependencyNode.hasAttribute("babylonTransparencyMode") == false)
{
  MGlobal.executeCommand($"addAttr -ln \"babylonTransparencyMode\" -nn \"Transparency Mode\" - at \"enum\" -en \"Opaque:Cutoff:Blend:\"{babylonAttributesDependencyNode.name};");
}
// Init alpha mode value based on material opacity
if (babylonMaterial != null)
{
  MGlobal.executeCommand($"setAttr \"{babylonAttributesDependencyNode.name}.babylonTransparencyMode\" {babylonMaterial.transparencyMode};");
}
...

And if correct we MUST extends this behaviors to others attributes otherwise, the bug will appear again with backface culling and others.

But I continue to beleive that the right solution is to clear the attributes at the end of the export.

@dkyteasSG
Copy link
Contributor Author

I'll make the change.
I think I've already come across the culling issue. I'll verify it on Monday.

I agree that the newly created babylonAttributesDependencyNode instances should be deleted at the end

@dkyteasSG
Copy link
Contributor Author

dkyteasSG commented Nov 23, 2020

Looks like "babylonTransparencyMode" is set separately in each babylonMaterialNode class (babylonAiStandardSurfaceMaterialNode, babylonStandardMaterialNode, babylonStingrayPBSMaterialNode)

I need to understand exactly what's going on with these nodes or this issue will keep popping up.
I can also confirm that I have the same issue with backfaceCulling

@pandaGaume
Copy link
Contributor

At the end of the day, the purpose seems to let user update Babylon specifics value throught Maya UI (available after the first export - this is the same strategy applied on Max). On my opinion, it's introducing some new node into the DAG, and then modify the scene - so we may tell the user to do it explicitely if he/she want to persist his/her change into the scene - Also using MxNode for the purpose is kind of hack while the goal of these class is to write some "compute"code to process dependencies. Anyway, i do not know the history and motivation for this, but i'm sure it was for a good reason. We can therefore make sure that the code behaves identically, whether the nodes are present or not.

…iStandardSurfaceMaterialNode and babylonStingrayPBSMaterialNode
…bylonMaxSimultaneousLights" attributes for all 3 babylon material nodes
@dkyteasSG
Copy link
Contributor Author

I've pushed a couple of changes to ensure all attributes in the material nodes are initialised.
I think it makes sense to do it in the context of this PR.
This should make the exports consistent.

Copy link
Contributor

@pandaGaume pandaGaume left a comment

Choose a reason for hiding this comment

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

I wonder why you treated BackFaceCulling any differently? by setting it to 1 just after its creation.

Remove default "setattr" for "babylonBackfaceCulling"
@dkyteasSG
Copy link
Contributor Author

dkyteasSG commented Nov 23, 2020

That's the original code.
It's probably just an inconsistency.
I removed it. Doesn't seem to make a difference. Looks like "true" is the default state anyway.

I think these changes have exposed a bug.
After the 1st export
MGlobal.executeCommand($"setAttr "{babylonAttributesDependencyNode.name}.babylonTransparencyMode"
{babylonMaterial.transparencyMode};");

in babylonStingrayPBSMaterialNode (line#69) is throwing an exception.
The other attributes do not.
babylonStandardMaterialNode does not have this issue either.

@Drigax
Copy link
Contributor

Drigax commented Nov 23, 2020

This UI was added due to #537

The problem we intended to solve is that Maya Arnold Materials don't have all the parameters that one may want to control when exporting to glTF, so we created that custom material node to contain the rest.

One restriction was that we were not able to hook into Maya material creation to initialize the node at edit time, so instead we initalized the nodes at export time, which is causing all this UX madness with nodes appearing out of the blue to the end user.

The current issue this PR is addressing appears to be that the initialized supplemental nodes aren't properly configured, the current fix seems sufficient to change that behavior.

Deleting the nodes after export may not be the best idea either, the workflow for setting up the nodes isn't particularly intuitive given our multiple node types, I don't really like this solution, but I definitely don't like the idea of all this occuring completely opaque to the user. at the very least here, we're leaving a record of what we're adding to the material.

This ties back to our conversation last week @pandaGaume regarding how difficult it can be to augment the DCC tool with additional functionality, the currently implemented solution is pretty bad, to be honest.

I think a better, less disruptive workflow is to use Maya material custom attributes in, instead of MxNode graph objects. but that's more of a "burn this monstrosity down" long term solution.

In the meantime, this seems alright as a fix.

@dkyteasSG
Copy link
Contributor Author

Thanks for the information @Drigax.
Anyone has any feedback about the babylonStingrayPBSMaterialNode.babylonTransparencyMode?
Should I disable its initialisation and raise it as a separate issue?

@pandaGaume
Copy link
Contributor

could you send me a scene that reproduce the issue ?

@dkyteasSG
Copy link
Contributor Author

Here's a simple scene that can be used to reproduce the issue.
1st export should work. The rest should fail.

@dkyteasSG
Copy link
Contributor Author

Some additional info.
I tried running the following MEL command from the Maya script editor after the first export:
setAttr "babylonStingrayPBSMaterialNode1.babylonTransparencyMode" 2

It came back with:
Error: line 1: setAttr: The attribute 'babylonStingrayPBSMaterialNode1.babylonTransparencyMode' is locked or connected and cannot be modified.

The other attributes don't have this issue

@dkyteasSG
Copy link
Contributor Author

Turns out that babylonTransparencyMode for PBS materials was explicitly locked after its creation.
I couldn't see a reason for this (no other custom attribute is locked) so I removed the MEL command.

@pandaGaume
Copy link
Contributor

I beleive the lock issue is wider, because once created, any of the attribute could be locked by the user (when lock the material itself). This mean we maybe put a logic in place for our own attribute to ensure there are not lock before set, and put them back to the previous status after the set.
maybe use something like:
$"listAttr -locked {dag}"
and test the result to see if the attribute is in...

@dkyteasSG
Copy link
Contributor Author

If that's the case, the lock status must also be taken into account in the Init() function of the babylon material nodes, as it calls "setAttr" for multiple attributes.
Maybe what's really needed is a "set" function for each custom attribute that handles locking (and any other special condition).
i.e. babylonStingrayPBSMaterialNode::setBabylonTransparencyMode(int mode)
babylonStingrayPBSMaterialNode::setBabylonBackfaceCulling(bool backfaseCulling)
etc

@dkyteasSG
Copy link
Contributor Author

I've pushed a change with a setter function.
All three babylon material nodes have identical functions, save for Create().
Is it possible to have them inherit from a base class or is there some limitation from OpenMaya?

@pandaGaume
Copy link
Contributor

As far i know there is no restriction about having common super-class, nor an extension. We should introduce a common class such

public class BabylonMPxNode : MPxNode {
}

@dkyteasSG
Copy link
Contributor Author

I added 2 abstract classes:
babylonMPxNode which can be used for any custom node and babylonMaterialNodeBase (deriving from babylonMPxNode) as the base for material nodes.
Because transparencyMode is not a property of BabylonMaterial, I set it and initialise it in the derived material nodes.

Copy link
Contributor

@pandaGaume pandaGaume left a comment

Choose a reason for hiding this comment

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

Maya/Exporter/PluginNodes/babylonMPxNode .cs
remove white space before extension please.

Copy link
Contributor

@Drigax Drigax 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 improvements, we greatly appreciate the discussion and contributions :)

@Drigax Drigax merged commit b04fe21 into BabylonJS:master Dec 4, 2020
@dkyteasSG dkyteasSG deleted the dkyteasSG/material_transparency branch December 4, 2020 10:40
renjianqiang pushed a commit to renjianqiang/Exporters that referenced this pull request Dec 18, 2020
BabylonJS#897)

* Make sure "babylonTransparencyMode" attribute is correctly initialised

* Refactor attribute initialisation fix

* Correctly initialise "babylonTransparencyMode" attribute for babylonAiStandardSurfaceMaterialNode and babylonStingrayPBSMaterialNode

* Correctly initialize "babylonBackfaceCulling", "babylonUnlit" and "babylonMaxSimultaneousLights" attributes for all 3 babylon material nodes

* Fix "setattr" command for bool attributes of babylon material nodes.
Remove default "setattr" for "babylonBackfaceCulling"

* Do not lock babylonTransparencyMode attribute for StingrayPBS materials.

* Set attributes for babylon materials through a setter function.
Handle locked attributes.

* Add babylonMPxNode and babylonMaterialNodeBase

* Remove whitespace
renjianqiang added a commit to renjianqiang/Exporters that referenced this pull request Dec 18, 2020
renjianqiang added a commit to renjianqiang/Exporters that referenced this pull request Dec 18, 2020
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.

subsequent exports have different material.
3 participants