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

Added support for explicitly setting the glTF "doubleSided" material … #258

Closed

Conversation

atteneder
Copy link
Contributor

…property.

This is done via custom Blender material property. Works for import and export and adds a UI Panel in the material property tab for easy (un-)checking.

fixes #154

@CLAassistant
Copy link

CLAassistant commented Jan 27, 2019

CLA assistant check
All committers have signed the CLA.

@julienduroure julienduroure added enhancement New feature or request exporter This involves or affects the export process importer This involves or affects the import process labels Jan 27, 2019
@atteneder
Copy link
Contributor Author

Note:
I added a custom property to Blender's material ("gltf_double_sided"). One could argue that this needs consideration, since it is stored in the blend files permanently and needs historic backwards compatibility. I have no idea if this is the proper way of doing this and if there's a guideline/checklist for adding custom data.

@ghost ghost requested a review from UX3D-becher January 28, 2019 10:42
@mholthausen
Copy link

I think this conflict is related to what I expected, when I have tested the magnificent add-on: the assigned material (e.g. diffuse color) isn't exported into the glTF/glb-File, although the "Export Materials" is checked in the export menu.

…property.

This is done via custom Blender material property. Works for import and export and adds a UI Panel in the material property tab for easy (un-)checking.
@atteneder
Copy link
Contributor Author

I think this conflict is related to what I expected, when I have tested the magnificent add-on: the assigned material (e.g. diffuse color) isn't exported into the glTF/glb-File, although the "Export Materials" is checked in the export menu.

I doubt that this is related to your problem since I cannot reproduce. Is the error happening on master as well? If not, could you provide me with your setup, steps and test data?

@mholthausen
Copy link

I think this conflict is related to what I expected, when I have tested the magnificent add-on: the assigned material (e.g. diffuse color) isn't exported into the glTF/glb-File, although the "Export Materials" is checked in the export menu.

I doubt that this is related to your problem since I cannot reproduce. Is the error happening on master as well? If not, could you provide me with your setup, steps and test data?

It's the current Blender 2.80 Beta downloaded ob blender.org
Date: 2019-01-27 19:17
Hash: 690478027bd7
Branch: blender2.7

Running on Linux Mint 19

The worklfow consists of

  1. Importing a shapefile into Blender with the BlenderGIS-Plugin (https://github.com/domlysz/BlenderGIS/tree/Blender28) as mesh where the normals of some triangles are up and some others are down

  2. Assigning a material and color to the mesh and checking the "glTF double sided" of the box in the "Material" pane

  3. Checking "Double Sided" in Normals section of "Object Data" pane

  4. Exporting the scene as glb

  5. Uploading the scene into the glTF-Viewer (https://gltf-viewer.donmccurdy.com/) and the assigned color is missing

Behavoiur occurs in master of gltF-Blender-IO, too.
In BLender 2.79 with the current master of glTF-Blender-Exporter works like expected (without double sided, of course)

So, i guess it's a miscommunication between Blender 2.8 Beta and glTF-Blender-IO for 2.8

@atteneder
Copy link
Contributor Author

Behavoiur occurs in master of gltF-Blender-IO, too.

Thanks. In that case, you should create a proper issue with a copy of your description. It's not related to this PR at all, as far as I can tell.

@donmccurdy
Copy link
Contributor

Note that #255 was just merged, allowing doubleSided to be exported from the old custom nodes. We (intentionally) do not create those old nodes when importing glTF models, but at least it provides an escape hatch for exporting the property.

If I'm reading correctly, this option appears in the main Blender materials panel, not the materials panel at the export step? Back when glTF-Blender-IO required an opt-in installation by users, I think this fix would have been OK. However, as the addon comes enabled by default now, we're essentially adding new UI for all Blender users by doing this. As Blender already has (multiple) similar options, I'm not sure we can justify it.

A more robust fix, perhaps, would be to prescan all Blender meshes for a double sided flag, and then set material.doubleSided for materials on those meshes. A material may be reused by a mesh that isn't double sided, and ideally that would be handled too, but some false positives/negatives may be OK for an initial fix.

@atteneder
Copy link
Contributor Author

atteneder commented Jan 29, 2019

Note that #255 was just merged, allowing doubleSided to be exported from the old custom nodes. We (intentionally) do not create those old nodes when importing glTF models, but at least it provides an escape hatch for exporting the property.

I noticed, but I too don't want to use the obsolete custom nodes.

If I'm reading correctly, this option appears in the main Blender materials panel, not the materials panel at the export step? Back when glTF-Blender-IO required an opt-in installation by users, I think this fix would have been OK. However, as the addon comes enabled by default now, we're essentially adding new UI for all Blender users by doing this. As Blender already has (multiple) similar options, I'm not sure we can justify it.

I'm not sure either :)

A more robust fix, perhaps, would be to prescan all Blender meshes for a double sided flag, and then set material.doubleSided for materials on those meshes. A material may be reused by a mesh that isn't double sided, and ideally that would be handled too, but some false positives/negatives may be OK for an initial fix.

Good idea. I thought about it briefly, but I wanted a quick solution without digging too much into the addon's code.

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

I'm in agreement with @donmccurdy that we should not be adding new options to the materials panel for all users. Looking for Blender's own double-sided lighting flag is the better choice.

@donmccurdy donmccurdy added this to General in Active Feb 14, 2019
@donmccurdy donmccurdy moved this from General to In Progress in Active Feb 14, 2019
@emackey
Copy link
Member

emackey commented Feb 20, 2019

Closing this in favor of the solution in #304.

@atteneder Sorry we couldn't take this one. There are a number of open issues remaining in this project, so we'd be glad to see more PRs with fixes, we appreciate them!

@emackey emackey closed this Feb 20, 2019
@donmccurdy donmccurdy moved this from In Progress to Done in Active Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exporter This involves or affects the export process importer This involves or affects the import process
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Checking Double Sided doesn't export as double-sided geometry
6 participants