Skip to content

Re-add tex deduplication by value - #51

Merged
EliCDavis merged 2 commits into
EliCDavis:mainfrom
alexykot:feature/convert-comparison
Dec 18, 2024
Merged

Re-add tex deduplication by value#51
EliCDavis merged 2 commits into
EliCDavis:mainfrom
alexykot:feature/convert-comparison

Conversation

@alexykot

Copy link
Copy Markdown
Contributor

I think converting texture deduplication to pointer-comparison pattern was a mistake.

Effectively, that pattern forced the real value deduplication onto the client, which is what I discovered when I tried to use the latest build of the library.


I think this using pointers is a wrong approach in principle. This approach actually means no real deduplication within the library, because if the pointers are pointing to the same object - deduplication was already done by the client. So the heavy lifting was done, and the all that library is doing is not screwing it all up, which is not an achievement.

I think the library should in fact do this heavy lifting and do real deduplication - compare objects by values and reuse the same object indexes in GLTF if the values are the same, even if the actual instances in memory are different.

With meshes the approach was different purely because of technical limitations - comparing large meshes directly by vector attribute values is prohibitively expensive. So that was pushed onto the client where the logic and metadata around the scene creation allow to avoid direct comparisons.

For all other objects - comparison is not technically difficult and should be done by the library.


I've kept pointer comparison as well as a shortcut, since it's already implemented and the API adjusted to support it - it's not worth rolling back. But I've added value-based deduplication alongside, and distributed comparison logic into appropriate equal() helper methods.

@alexykot
alexykot force-pushed the feature/convert-comparison branch from 6f807f5 to b4e2959 Compare December 17, 2024 22:59

@EliCDavis EliCDavis left a comment

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.

Thanks for this. I like the idea of helping people make a more efficient file size with a small sacrifice in performance, while providing the ability to maintain runtime performance utilizing pointers. Seems like the best of both worlds.

Comment thread formats/gltf/structure.go
Comment on lines +35 to +39
for key, val := range s.Extensions {
if other.Extensions[key] != val {
return false
}
}

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.

There is a general bug in this code due to the fact that map[thing] will return the default value for the value type of map. In this case, nil. We need to make sure that other.Extras contains the key as well. Or we can get some false positive situations

It might be worth considering the use of reflect.DeepEqual for comparisons of all map types. If people are truly concerned about performance, they can always take the time to satisfy the pointer shortcut we provide.

https://go.dev/play/p/TnjBO6Ub-yp

package main

import "fmt"

func equal(a, b map[string]any) bool {
	if len(a) != len(b) {
		return false
	}

	for key, val := range a {
		if b[key] != val {
			return false
		}
	}

	return true
}

func main() {
	a := map[string]any{
		"a": 1,
		"b": nil,
	}
	b := map[string]any{
		"a": 1,
		"c": 2,
	}

	fmt.Println(equal(a, b)) // Prints true
}

@alexykot alexykot Dec 18, 2024

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.

Ah, that one is easy to fix, I've pushed an update.

I'm somewhat surprised that any == any comparison works at all to be honest, and I don't really like this map[string]any type shape, it's very amorphous.

I don't think reflect is necessary here, I've pushed #52 as an example of how extensions could be made typed and explicitly comparable. This would avoid having any there at all, and still leave it flexible enough to permit custom extensions. They would just need to implement the interface.

@alexykot alexykot changed the title Readd tex deduplication by value Re-add tex deduplication by value Dec 18, 2024
@alexykot
alexykot requested a review from EliCDavis December 18, 2024 09:51
@EliCDavis
EliCDavis merged commit 3b39e14 into EliCDavis:main Dec 18, 2024
@alexykot
alexykot deleted the feature/convert-comparison branch December 18, 2024 17:07
kalebpace pushed a commit to kalebpace/polyform that referenced this pull request Jan 20, 2025
* Readd tex deduplication by value

* Check for key existence
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