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

Material alpha-blending modes #15557

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@SaracenOne
Copy link
Contributor

commented May 14, 2019

This PR adds support for specifiable blend modes to HF materials. The current implementation only looks at the albedo alpha channel and makes a judgement on the blend mode based on whether it has A. no alpha at all for opaque, B. 1-bit of alpha for masked transparency, or C. 8-bit alpha for full translucency. This implementation approaches it kind of like a fallback: by default, it is specified as using blend mode, meaning it will behave exactly as the system currently does, reading the properties of the albedo texture's alpha and choosing the most appropriate alpha blend, however, the other two modes allow you to explicitly specify the usage of treating the alpha as cutoff regardless of whether it uses 8bits of alpha, or opaque which will ignore the alpha entirely and render it as full opaque. While it is not 100% clear whether this fallback mechanic is the right way to go about this, whether this should instead allow the user to EXPLICITLY force a particular blend mode (I don't know why this ever might be necessary, disabling shadow casting is one, but this would be better implemented as its own setting), this functionality is needed at least to retain greater compliance with the GLTF spec, as can be seen here.

In order to test this, enter your local host and enter coordinates /8000,8000,8000, run the script (https://raw.githubusercontent.com/highfidelity/hifi_tests/master/tests/content/entity/model/modelReaders/gltfReader/testRecursive.js), and observe the alphaBlendModeTest. The following should look like this:
alpha

It supports all the correct blending modes, although specifiable cutoff is not currently supported. Explicit specification of cull mode will also be required to be fully compliant with the spec.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

Android build is available here. Quest build is available here

@SamGondelman
Copy link
Contributor

left a comment

I don't think we can call this feature really complete until we also add the ability to specify the alphaCutoff.

Also, does FBX/OBJ support a similar concept?

Show resolved Hide resolved libraries/fbx/src/GLTFSerializer.cpp Outdated
Show resolved Hide resolved libraries/fbx/src/GLTFSerializer.h Outdated
Show resolved Hide resolved libraries/graphics-scripting/src/graphics-scripting/Forward.h Outdated
Show resolved Hide resolved libraries/graphics/src/graphics/Material.h Outdated
Show resolved Hide resolved libraries/graphics/src/graphics/Material.h Outdated
Show resolved Hide resolved libraries/hfm/src/hfm/HFM.h Outdated
@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Android build is available here. Quest build is available here

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

SaracenOne added some commits Apr 29, 2019

Support for alpha mode.
# Conflicts:
#	libraries/fbx/src/GLTFSerializer.cpp

@SaracenOne SaracenOne force-pushed the SaracenOne:blend branch from 0d41f8a to 43e8c9b May 31, 2019

@SaracenOne

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Now with alphaCutoff
hifi-snap-by--on-2019-05-31_05-04-59

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Android build is available here. Quest build is available here

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Show resolved Hide resolved libraries/fbx/src/GLTFSerializer.cpp Outdated

objMaterial.opacity,
hfm::AlphaMode::HFM_BLEND,
0.5f);

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

Why always blend and 0.5?

This comment has been minimized.

Copy link
@SaracenOne

SaracenOne Jun 3, 2019

Author Contributor

The reason for this is the current implementation is built around a fallback mechanism, where it still takes the state of the albodo's opacity map into account when choosing an alpha mode: opaque, 1bit, or 8bit alpha, but setting the alpha mode basically limits the alpha mode you're allowed to fall back to, with opaque being the most restrictive. It basically allows you to force the renderer to ignore the presence or properties of an alpha channel. 0.5 is also just the default alpha cutoff range.

@@ -363,6 +363,8 @@ namespace scriptable {
obj.setProperty("name", material.name);
obj.setProperty("model", material.model);

obj.setProperty("alphaMode", Material::alphaModeAsString(material.alphaMode));

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

I think this should also have a property fallthrough and default and everything and be down by where you did alphaCutoff

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

Hm actually I guess fallthrough doesn't make sense for this?

@@ -21,6 +21,8 @@

scriptable::ScriptableMaterial& scriptable::ScriptableMaterial::operator=(const scriptable::ScriptableMaterial& material) {
name = material.name;
alphaMode = material.alphaMode;
alphaCutoff = material.alphaCutoff;

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

Can you put these in between opacity and roughness, here and everywhere else, just to group things more logically?

@@ -320,6 +320,44 @@ class Material {
const std::string& getName() const { return _name; }
void setName(const std::string& name) { _name = name; }

enum MaterialAlphaMode
{

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

enum MaterialAlphaMode {

};

static MaterialAlphaMode alphaModeFromString(const QString& alphaMode) {
if (alphaMode.toLower() == "blend") {

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

Could we still make both of these methods use std::string? Also you could just compute alphaMode.toLower() once at the top here.

}
return GLTFMaterialAlphaMode::OPAQUE;
return hfm::AlphaMode::HFM_OPAQUE;

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

This defaults to opaque, but elsewhere we default to blend. Does that matter? Which matches the current behavior?

HFM_BLEND = 0,
HFM_MASK,
HFM_OPAQUE,
};

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

It would still be great if we could avoid defining these in multiple places.

@@ -114,6 +114,9 @@ NetworkMaterialResource::ParsedMaterials NetworkMaterialResource::parseJSONMater
* @property {string} model="hifi_pbr" - Different material models support different properties and rendering modes.
* Supported models are: "hifi_pbr"
* @property {string} name="" - A name for the material. Supported by all material models.
* @property {string} alphaMode="" - The alpha mode for the material. Can be <code>"BLEND"</code>, <code>"MASK"</code>, or <code>"OPAQUE"</code>.

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

Should these be lowercase?

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

This is also "hifi_pbr" only.

@@ -39,8 +39,9 @@ void main(void) {
<$fetchMaterialTexturesCoord0(matKey, _texCoord0, albedoTex, roughnessTex, _SCRIBE_NULL, metallicTex, emissiveTex)$>
<$fetchMaterialTexturesCoord1(matKey, _texCoord1, occlusionTex)$>

float alphaCutoff = getMaterialAlphaCutoff(mat);

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman May 31, 2019

Contributor

tabs in these shaders

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

@hifi-gustavo

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Android build is available here. Quest build is available here

@SamGondelman
Copy link
Contributor

left a comment

build errors:
Material.h:318: type qualifier ignored on return type
HFM.h:165, HFM.h:178, HFM.h:190: initialization order of opacity and alphaMode



if (material.defined["alphaMode"]) {
hfmMat.alphaMode = static_cast<graphics::Material::AlphaMode>(material.alphaMode);

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

Is this cast necessary? Could getMaterialAlphaMode just return a graphics::Material::AlphaMode?

@@ -283,6 +283,45 @@ class Material {
void setOpacity(float opacity);
float getOpacity() const { return _opacity; }

enum AlphaMode
{

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

on one line: enum AlphaMode {

@@ -114,6 +114,9 @@ NetworkMaterialResource::ParsedMaterials NetworkMaterialResource::parseJSONMater
* @property {string} model="hifi_pbr" - Different material models support different properties and rendering modes.
* Supported models are: "hifi_pbr"
* @property {string} name="" - A name for the material. Supported by all material models.
* @property {string} alphaMode="" - The alpha mode for the material. Can be <code>"blend"</code>, <code>"mask"</code>, or <code>"opaque"</code>. "hifi_pbr" model only.

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

This also supports fallthrough

@@ -399,7 +403,7 @@ std::pair<std::string, std::shared_ptr<NetworkMaterial>> NetworkMaterialResource
material->setPropertyDoesFallthrough(graphics::Material::ExtraFlagBit::LIGHTMAP_PARAMS);
}
}
// TODO: implement lightmapParams
// TODO: implement materialParams

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

This was supposed to say lightmapParams, and then can you put back the TODO for materialParams below?

@@ -580,6 +603,8 @@ NetworkMaterial::NetworkMaterial(const HFMMaterial& material, const QUrl& textur
graphics::Material(*material._material)
{
_name = material.name.toStdString();
setAlphaMode(material.alphaMode);

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

Should this happen later, particularly after we've set the albedoTexture?

if (alphaChannelSetMaterial) {
alphaChannelSetMaterial->resetOpacityMap(alphaMode);
schemaKey.setOpacityMaskMap(material->getKey().isOpacityMaskMap());
schemaKey.setTranslucentMap(material->getKey().isTranslucentMap());

This comment has been minimized.

Copy link
@SamGondelman

SamGondelman Jun 3, 2019

Contributor

Can you explain this? My understanding is: we need to do this here and in the albedo map case, because we don't know which we will hit first? A comment to that effect would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.