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

Adding the KHR_materials_iridescence effect #2027

Merged
merged 88 commits into from
Jul 25, 2022

Conversation

UX3D-nopper
Copy link
Contributor

Replaces #1742

UX3D-nopper and others added 30 commits January 17, 2020 08:11
@PascalSchoen
Copy link
Contributor

PascalSchoen commented Apr 14, 2022

@sebavan this is defined in the specular extension and adopted by this extension (see KHR_materials_specular #Implementation).

The issue is, that if we simply use 1 - iridescence_fresnel for the weight of the base, you get inverse colors there. If you don't weight base at all, you gain energy. I updated the PR with the same description and a reference to KHR_materials_specular.

Another approach would've been to use the grayscale representation of iridescence_fresnel as this would reduce the energy-loss but could gain a bit of energy as well. It was thus decided by the group to use the approach, that is currently documented.

@emackey
Copy link
Member

emackey commented Apr 18, 2022

On the call today it was suggested we rename "iridescenceIOR" to "iridescenceIor" to fix the camelCase. @lexaknyazev has some additional nonbreaking corrections to some of the description fields, to align them with language used in the core spec.

@sebavan
Copy link
Contributor

sebavan commented Apr 18, 2022

All good for me I changed it in Babylon and it should be up in the next couple hours :-)

@UX3D-nopper
Copy link
Contributor Author

We can also make the changes in Gestaltor.

@UX3D-nopper
Copy link
Contributor Author

However, did we think about the camel case rule for OpenGL. or OpenXR?
IOR is Index Of Refraction and IOR in short. So, in this case, it should stay iridescenceIOR, correct?
Otherwise it should be OpenGl or OpenXr, right?

@lexaknyazev
Copy link
Member

lexaknyazev commented Apr 19, 2022

@UX3D-nopper
Existing glTF JSON properties follow lowerCamelCase convention. The KHR_materials_ior extension did not make an exception for its ior property despite the latter being an acronym. So for consistency, we should call iridescence IOR as iridescenceIor.

@UX3D-nopper
Copy link
Contributor Author

UX3D-nopper commented Apr 19, 2022

So the rule is like the one here: https://google.github.io/styleguide/jsguide.html#naming-camel-case-defined
Or do we have a better document for this?

@UX3D-kanzler
Copy link
Contributor

I have opened a pull request for the sample models (iridescenceIOR->iridescenceIor): KhronosGroup/glTF-Sample-Models#349

@donmccurdy
Copy link
Contributor

PR for gltf-transform: donmccurdy/glTF-Transform#570

@echadwick-artist
Copy link
Contributor

It would be helpful if the code block in the Extending Materials section included each of the Properties, to illustrate all their formatting, e.g.:

{
    "materials": [
        {
            "extensions": {
                "KHR_materials_iridescence": {
                    "iridescenceFactor": 1.0,
                    "iridescenceTexture": {
                      "index": 0
                    },
                    "iridescenceIor": 1.3,
                    "iridescenceThicknessMinimum": 200.0,
                    "iridescenceThicknessMaximum": 400.0,
                    "iridescenceThicknessTexture": {
                      "index": 0
                    }
                }
            }
        }
    ]
}

@emackey
Copy link
Member

emackey commented Jul 25, 2022

This was ratified by the Khronos Board on July 22, 2022.

@emackey emackey merged commit 1856425 into KhronosGroup:main Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet