returns unmodifiable sets

[rvandoosselaer]

Returns an unmodifiable set when retrieving the available animation names and available animation clips instead of an unmodifiable collection. fixes: #1070

[pspeed42]

These changes create a LOT of garbage. The animation names one does so unnecessarily since it was already a set. No need to create a new one just to again wrap it in an unmodifiable wrapper.

I'm kind of against the clips -> Set change just because the data doesn't support it. There is no good reason to return Set here when a documentation change would do and not have as much garbage.

[rvandoosselaer]

I agree with you concerning the animation names, I did wrap them in an UnmodifiableSet to keep in line with the current implementation but I indeed used an unnecessary wrapper set. Just need to be careful that the returned object is wrapped in a new object (be it collection or set) so the underlying map isn't modified.

For the Collection vs Set discussion, I don't really have a strong opinion. But a Set makes senses imo.

[pspeed42]

For AnimClip objects, a set would make sense if it were already a set. But it isn't. Set is "nice to have" because it auto-documents. That's it. In this case, the same can be achieved by mentioning something in the javadoc... though I guess no one will ever really care because the names are already a set.

Also, while today's implementation may be one-to-one mapping between name and AnimClip instance, that is not a requirement of this part of the API and I think it's a mistake to reflect it here for "no good reason".

[stephengold]

If it's not possible to convert Collection.values() to a Set without creating extra garbage, then we should skip the getAnimClips() half of the change. The change to getAnimClipsNames() still seems worthwhile.

[rvandoosselaer]

I concur with avove remarks.
I’ll revert the change for the Collection.values() part, leaving it an unmodifiable collection. Effectively removing the overhead of wrapping it in a hashset and again wrapping it in an unmodifiable set, but adapting the javadoc as Paul mentioned to better describe the behaviour.

Is this an ok solution?

[stephengold]

ok

[Ali-RS]

@stephengold wasn't getAnimClips supposed to reverted back to return Collection ?

[pspeed42]

Yep... this wasn't ready to apply yet, unfortunately. Still has the crappy duplication of hashset every time getAnimClips() is called... for no good reason other than "let's return a set"... which itself is also not a good reason.

[rvandoosselaer]

I was/am on a short holiday during the easter weekend with no access to my pc. I'll create a new PR this evening with the discussed changes in it.

[stephengold]

Oops! My bad.