Skip to content

Add alphaMode and alphaCutOff material settings - #41

Merged
EliCDavis merged 7 commits into
EliCDavis:mainfrom
alexykot:feature/add-material-alphamode
Nov 28, 2024
Merged

Add alphaMode and alphaCutOff material settings#41
EliCDavis merged 7 commits into
EliCDavis:mainfrom
alexykot:feature/add-material-alphamode

Conversation

@alexykot

@alexykot alexykot commented Nov 27, 2024

Copy link
Copy Markdown
Contributor

This adds a way to set alphaMode for materials in GLTF. Creating transparent materials with GLTF is impossible without setting alphaMod=BLEND.

This seems to be very simple to implement, but I'm not sure what is the long term plan on aligning different interfaces and formats. If this approach is fine - I'll add tests and finalise the PR. If not - let me know what would be better way. I need this feature and happy to implement it in whatever way is best.


Updated to full implementation, ready for review/merge.

Comment thread formats/gltf/model.go Outdated
Comment thread formats/gltf/writer.go Outdated
@EliCDavis

EliCDavis commented Nov 28, 2024

Copy link
Copy Markdown
Owner

Good catch! This fix is fine, and I am happy to merge. Make sure this is the only change you need to make for your own purposes. I've been able to achieve transparency in three.js and babylon renderers while this bug has existed.

https://github.com/EliCDavis/polyform/blob/main/examples/ufo/main.go#L347-L380

EDIT:
Could your changes also go ahead and include the use of AlphaCutoff

image

@alexykot

alexykot commented Nov 28, 2024

Copy link
Copy Markdown
Contributor Author

Cool, I've switched to pointers and added alphaCutOff as requested. Also added relevant writer tests.

Yes indeed, the material settings of the UFO model work in Babylon and Three.js. I copied over the parts of GLTF JSON and it did render transparency. I'm not sure why tho, if I read the spec correctly - it should not work, as default should be OPAQUE even if alphaMode is not explicitly set.

Btw, I also tested against 3dviewer.net and it did not work without BLEND, so there is clearly a disagreement between renderer implementations.

Anyway, it's part of the spec, so should exist, and now it does 🙂

@alexykot
alexykot requested a review from EliCDavis November 28, 2024 07:11
Comment thread formats/gltf/writer.go Outdated
Comment on lines +372 to +375
var alphaCutOff *float64
if mat.AlphaMode != nil {
alphaCutOff = mat.AlphaCutoff // alphaCutOff should only be set if the alphaMode is set
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I wanted your thoughts on this implementation. I could see users getting confused by us silently not setting alphaCutOff even though they included it. Given it's illegal, it might warrant a panic.

Suggested change
var alphaCutOff *float64
if mat.AlphaMode != nil {
alphaCutOff = mat.AlphaCutoff // alphaCutOff should only be set if the alphaMode is set
}
alphaCutOff := mat.AlphaCutoff
if mat.AlphaMode == nil && mat.AlphaCutoff != nil {
// https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#_material_alphacutoff
panic("When alphaMode is not defined, alphaCutOff MUST NOT be defined")
}

An alternative approach to this would be preventing the user from ever making an illegal move through the introduction of a new field to the PolyformMaterial that encapsulates the alpha logic.

type PolyformMaterialAlpha interface {
    Set(mat PbrMetallicRoughness)
}

type PolyformMaterialAlphaBlend struct {
     CutOff *float64
}

func (pmab PolyformMaterialAlphaBlend) Set(mat PbrMetallicRoughness) {
    mat.AlphaMode = MaterialAlphaMode_BLEND
    mat.AlphaCutoff = pmab.CutOff
}

Appreciate your close attention to the spec. I totally missed that bit of logic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but are you sure this is the best way to solve it?

I think, if we treat it as an invalid input - it warrants an error to be bubbled up and returned from the WriteText/WriteBinary top level functions. Inducing a panic crash seems a bit harsh reaction for an invalid user input 😃

Also, your suggestion will prevent user from setting wrong parameters, but it also makes the interface quite less ergonomic and implementation - more complex.

I think we should strike a balance here between being careless on one hand and overbearing on the other.

My first implementation was too careless, let me try adding an error return. Let me know what you think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made an update, let me know what you think of this version.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this is a good compromise! Thanks again for this fix.

@alexykot
alexykot requested a review from EliCDavis November 28, 2024 18:19
@alexykot alexykot changed the title Add sample implementation of alphaMode material setting Add alphaMode and alphaCutOff material settings Nov 28, 2024
@EliCDavis
EliCDavis merged commit 0e8594e into EliCDavis:main Nov 28, 2024
@alexykot
alexykot deleted the feature/add-material-alphamode branch November 28, 2024 21:18
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.

2 participants