[varLib] Reactivate shared points logic, bugfixes #1009
Conversation
|
@jenskutilek you know that you don't need to wait for the CI, but can run the test locally before pushing? |
|
Thanks! I didn’t know how to run them all at once. I ran each test on its own and missed some other ones that failed ;) |
| @@ -514,8 +515,7 @@ def compileTupleVariationStore(variations, pointCount, | |||
| # Apple will likely fix this in macOS 10.13. But for the time being, | |||
| # we never emit shared points although the result would be more compact. | |||
There was a problem hiding this comment.
Shouldn't you also update the comment here?
|
I've reviewed this and looks fine, though it doesn't look like the code finds the optimal set of points to share, right? Isn't it always sharing the "all points"? I thought the "all points" case doesn't even need to be encoded? I need to go read the spec again. |
|
Checked the spec again. I think we really want to find the point set that is referenced most and make that shared... |
|
I can take this over if there's no rush. Otherwise, feel free to give it a try. The IUP optimizer code then also needs to be updated to take into account the point sharing for maximal optimization. |
|
You’re right, as it is, this checks only if all or no points should be shared. Sharing all points still saves some space in the file, because the number of points has to be stored only once, not for each variation. At least in my current test project (10 masters), that shaved off a few kB. If determining the optimal point set to share is connected to the IUP optimization, that could really become a hard optimization problem. Sometimes a few points unneccessary for IUP could be added in a variation, so the point set gets shareable ... One quick optimization could be to check not for all points, but for the points actually used. If the IUP optimization happened to select the same point set for all variations, it could save some space. Not sure how likely this is to happen just by accident. |
|
I think this is low in my priorities right now, so if you could take over, it would be great. No need to hurry. |
Yes, that was what I had in mind. Ie. optimize each deltaset IUP separately, then see what point set saves most if shared. |
|
Jens, what's the status of this? |
|
In my opinion, it could be merged as is. I don’t have time to work on the mentioned optimizations at the moment, but the current status would save some file size. |
|
A small improvement on this is here: #1090 |
Related to issue #1001, this reactivates the sharing of point indices in TupleVariations if doing so saves file size.
It also fixes the bug leading to invalid gvar data described in the issue.