[api-minor] Simplify min/max computations in constructPath (bug 1135277) #14796
Conversation
Snuffleupagus
changed the title
src/core/evaluator.js
Outdated
| // If the current matrix (when drawing) is a scaling one | ||
| // then min/max can easily be computed in using those values. |
There was a problem hiding this comment.
Grammar nits (assuming I'm understanding the code properly):
// If the current matrix (when drawing) has a scaling of one
// then min/max can easily be computed by using those values.
src/display/canvas.js
Outdated
| Util.scaleMinMax(currentTransform, minMaxForBezier); | ||
| current.minX = Math.min(current.minX, minMaxForBezier[0]); | ||
| current.maxX = Math.max(current.maxX, minMaxForBezier[1]); | ||
| current.minY = Math.min(current.minY, minMaxForBezier[2]); | ||
| current.maxY = Math.max(current.maxY, [3]); |
There was a problem hiding this comment.
This [3]-part looks like a simple typo, right?
If the patch passes all tests like this, then we'd probably want some better test-coverage here...
How about instead adding a new method for this code, rather than defining it inline like this?
Maybe something like the following (with perhaps a better name), placed e.g. somewhere around here:
updateScalingPathMinMax(transform, minMax) [
Util.scaleMinMax(transform, minMax):
this.minX = Math.min(this.minX, minMax[0]);
this.maxX = Math.max(this.maxX, minMax[1]);
this.minY = Math.min(this.minY, minMax[2]);
this.maxY = Math.max(this.maxY, minMax[3]);
}| minMax[0] = Math.min(minMax[0], args[0]); | ||
| minMax[1] = Math.max(minMax[1], args[0]); | ||
| minMax[2] = Math.min(minMax[2], args[1]); | ||
| minMax[3] = Math.max(minMax[3], args[1]); |
There was a problem hiding this comment.
Please add a break; here as well, to prevent a possible future bug if this code is ever moved around.
src/display/canvas.js
Outdated
| minMax[0] = Math.min(minMax[0], box[0], box[2]); | ||
| minMax[1] = Math.min(minMax[1], box[0], box[2]); | ||
| minMax[2] = Math.min(minMax[2], box[1], box[3]); | ||
| minMax[3] = Math.min(minMax[3], box[1], box[3]); |
There was a problem hiding this comment.
Are all of these lines actually supposed to use Math.min(...), or was there a bit too much copy-pasting happening here?
src/shared/util.js
Outdated
| // Apply a scaling matrix to some min/max values. | ||
| // If a scaling factor is negative then min and max must be | ||
| // swaped. |
There was a problem hiding this comment.
Maybe place this comment just above the method instead?
It isn't a crazy optimization, I just noticed that in different profiles it was the main bottleneck inside |
- most of the time the current transform is a scaling one (modulo translation), hence it's possible to avoid to apply the transform on each bbox and then apply it a posteriori; - compute the bbox when it's possible in the worker.
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/0e9e9a6889c0224/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/c4ea02a577d2911/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/c4ea02a577d2911/output.txt Total script time: 23.49 mins
Image differences available at: http://54.241.84.105:8877/c4ea02a577d2911/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/0e9e9a6889c0224/output.txt Total script time: 24.45 mins
Image differences available at: http://54.193.163.58:8877/0e9e9a6889c0224/reftest-analyzer.html#web=eq.log |
hence it's possible to avoid to apply the transform on each bbox and then apply
it a posteriori;