Deduplicate gltf materials during scene writing

[alexykot]

This is a new feature for GLTF writer, to deduplicate identical materials and only write the necessary number of unique materials, rather than a material per model. Different models using the same material will correctly reference the single instance of this material.

This improves size of the resulting GLTF/GLB file without changing the functionality in any way. This is adhering to GLTF spec - the separation of materials from models and explicit reference via material index encourages reuse of material objects imo.

[EliCDavis]

Suggested change

	func (mt *materialTracker) findExistingMaterialID(mat PolyformMaterial) (*int, bool) {
	for _, entry := range mt.entries {
	if entry.polyMaterial.equal(mat) {
	return &entry.index, true
	}
	}
	return nil, false
	}
	func (mt *materialTracker) findExistingMaterialID(mat PolyformMaterial) int {
	for _, entry := range mt.entries {
	if entry.polyMaterial.equal(mat) {
	return entry.index
	}
	}
	return -1
	}

[alexykot]

Hmm, why? There is no functional change here as far as I can see, beyond a micro-optimisation of returning one value instead of two. But I'm not sure it's worth it.

And this doesn't look Go-idiomatic to me. Is this your preferred style for this project?

[EliCDavis]

I can take it or leave it. Suggestion inspired by strings.Index. But if you prefer it that way it's fine

[alexykot]

Asked Claude about it. Prompt:

In the Go standard library - what style of item in the list presence check style is more prevalent - returning one value index: -1 if not found, or returning two values index: 0, found: false ?

His response:

Looking at the Go standard library, the two-value return pattern with a boolean flag is significantly more prevalent than returning -1 for "not found" cases. This pattern is consistently used across many standard library packages. Some examples:

strings.Index() returns -1 (single return)
map lookups return value, ok (two returns)
strings.Cut() returns text, ok (two returns)
Array/slice element type assertions return value, ok (two returns)
Type switches return value, ok (two returns)
Channel reads return value, ok (two returns)

The -1 convention in strings.Index() is actually somewhat of an outlier, likely kept for historical/compatibility reasons. For new code in Go, the two-value return with a boolean flag is considered more idiomatic.

---

So yeah, I suggest we leave it as is 🙂

[EliCDavis]

Suggested change

	if existingId, ok := w.matTracker.findExistingMaterialID(mat); ok {
	return existingId, nil
	}
	if existingId := w.matTracker.findExistingMaterialID(mat); existingId != -1 {
	return &existingId, nil
	}

[EliCDavis]

This is a great addition! One way to improve efficiency and avoid unnecessary checks is to change the signature of AddMaterial(mat PolyformMaterial) to take a pointer AddMaterial(mat *PolyformMaterial).

This way we can first check if the pointers are referencing the same thing, and if not, then we start doing our equality checks.

It's a breaking change to the API, but a very tiny one.

[alexykot]

This is a great addition! One way to improve efficiency and avoid unnecessary checks is to change the signature of AddMaterial(mat PolyformMaterial) to take a pointer AddMaterial(mat *PolyformMaterial).

This way we can first check if the pointers are referencing the same thing, and if not, then we start doing our equality checks.

It's a breaking change to the API, but a very tiny one.

Cool. I didn't want to break the API, but if you think it's ok - I'll add that.

[alexykot]

Addressed comments

[alexykot]

Hmm, why? There is no functional change here as far as I can see, beyond a micro-optimisation of returning one value instead of two. But I'm not sure it's worth it.

And this doesn't look Go-idiomatic to me. Is this your preferred style for this project?

[alexykot]

An argument can be made that Name doesn't matter. They are arbitrary and optional and thus - don't need to be checked. I opted to check this as well, but practically it's of no use for the rendering process, so we could omit it for brevity. Lmk if you think it should be omitted.

[EliCDavis]

Fair point. I can't foresee why someone would care about a name, but I also don't want to potentially mess up an export for someone who builds logic around the material definition/name.

I think if we do remove this name check, we should introduce a new argument in the NewWriter() function, which would be a WriterConfig struct. I think for the time being, it'll have one field that'd be like, OptimizeMaterials . But it will allow us to add more options over time without breaking the API. If OptimizeMaterials is set to true, we do everything you've created in this PR. If it's set to false, it doesn't do anything

type WriterConfig struct {
    OptimizeMaterials bool
}

I might do this anyway. It would be a valid spot to add a new field like PanicOnInvalidDefinition, which would have addressed yesterdays PR.

Lemme know your thoughts!

[alexykot]

I would leave it as it is implemented now.

I assume in 99.9% of cases OptimizeMaterials is the desired behavior anyway. There is a microscopic performance hit on having to run the deduplication, but it's negligible, while the benefit of reduced model size is material. I struggle to imagine a use case where duplicated materials would be preferable. So putting it behind a flag that needs to be manually set, just to have it set in 99.9% of the times would just create additional lines in the caller implementation for no good reason.

If for whatever reason the user does want to have duplicates - the current shape of implementation gives them that ability. Assign unique names to materials, and they will be duplicated.

[EliCDavis]

Fair point. I can't foresee why someone would care about a name, but I also don't want to potentially mess up an export for someone who builds logic around the material definition/name.

I think if we do remove this name check, we should introduce a new argument in the NewWriter() function, which would be a WriterConfig struct. I think for the time being, it'll have one field that'd be like, OptimizeMaterials . But it will allow us to add more options over time without breaking the API. If OptimizeMaterials is set to true, we do everything you've created in this PR. If it's set to false, it doesn't do anything

type WriterConfig struct {
    OptimizeMaterials bool
}

I might do this anyway. It would be a valid spot to add a new field like PanicOnInvalidDefinition, which would have addressed yesterdays PR.

Lemme know your thoughts!

[EliCDavis]

Suggested change

	r1, g1, b1, a1 := a.(color.Color).RGBA()
	r2, g2, b2, a2 := b.(color.Color).RGBA()
	r1, g1, b1, a1 := a.RGBA()
	r2, g2, b2, a2 := b.RGBA()

[EliCDavis]

Right now it's fine where it is if you want to go ahead and merge it.

[alexykot]

@EliCDavis I don't have write access to your repo, so I don't have a merge button to click 🙂

Can you push the button on your end please?