Group 'rectangle' operator into constructPath #4993
Conversation
|
Can you change it to add to add rectange as is, and expand it to path on the other side. See you are removing logic that prevents rectangle from disappearing in the canvas.js, I would like to see this logic in the constructPath. |
|
Also, change svg.js to not process rectangle operator. |
| self.buildPath(operatorList, OPS.lineTo, [x, y + height]); | ||
| self.buildPath(operatorList, OPS.lineTo, [x, y]); | ||
| self.buildPath(operatorList, OPS.closePath, []); | ||
| break; |
There was a problem hiding this comment.
shall be continue; to not add that to the operator list
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 1 Live output at: http://107.21.233.14:8877/251775e49b179a0/output.txt |
From: Bot.io (Linux)FailedFull output at http://107.21.233.14:8877/251775e49b179a0/output.txt Total script time: 3.34 mins
Image differences available at: http://107.21.233.14:8877/251775e49b179a0/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/e5941861c979c15/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/e5941861c979c15/output.txt Total script time: 3.33 mins
Image differences available at: http://107.22.172.223:8877/e5941861c979c15/reftest-analyzer.html#web=eq.log |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/35f9a06a7acefa6/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/35f9a06a7acefa6/output.txt Total script time: 0.73 mins Published
|
|
I can confirm Yury's observation. Page 13 of the Tracemonkey paper stops rendering: |
| var width = args[j++]; | ||
| var height = args[j++]; | ||
| var xw = x + width; | ||
| var yh = y + height; |
There was a problem hiding this comment.
move these two lines after if (height === 0) check
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.22.172.223:8877/09974790dba8432/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @yurydelendik received. Current queue size: 0 Live output at: http://107.21.233.14:8877/ee240ddda454488/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/09974790dba8432/output.txt Total script time: 21.24 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/ee240ddda454488/output.txt Total script time: 23.79 mins
|
Combine re element into constructPath
|
Thank you |
|
What was the motivation for this change? As far as I can tell it's a net pessimization. I'm looking at the operator lists for two large map PDFs and OPS.rectangle never appears in a sequence with any of the other path-building ops. |
|
@nnethercote, If I understand correctly, this was made so that there wouldn't be a need for extra This might be an optimization for SVG. Not sure for canvas. |
|
The constructPath operation optimizes contiguous sequences of path operations. It's worthwhile because these sequences are often long, and so storing them together can save space, and they can be processed more quickly in a group. But for path operations that occur in isolation, it's a pessimization -- they take up slightly more space and are slightly slower to process. In practice, they occur in clusters so it's worthwhile overall. However, The To be frank, this pull request is terrible. The description is empty, the commit message is absurdly short (and even uses "re" instead of "rect" or "rectangle"), there is no explanation of why the change is beneficial. It's not clear if it's an optimization or a functionality improvement, and if it is the former, there are no measurements. IMO it should not have been merged in this state. |
|
Hmm, I guess @yurydelendik can explain the motivation behind this in a better manner, since it was his suggestion. As for the PR, my bad. I will edit the name and description in a better way. |
pramodhkp
changed the title
pramodhkp
changed the title
This groups the 'rectangle' operator into the constructPath operations (moveto, lineto, etc.), eliminating the same.