delay match_aspect adjustment #7342
Conversation
|
Want to see how CI report looks, do additional local testing, and update docs to warn about |
| @@ -216,43 +216,24 @@ export class PlotCanvasView extends DOMView | |||
| follow_enabled = false | |||
| has_bounds = false | |||
|
|
|||
| unioned_bounds = | |||
|
|
|||
| r = null | |||
| if @model.plot.match_aspect != false and @frame._width.value != 0 and @frame._height.value != 0 | |||
| r = 1/@model.plot.aspect_scale*(@frame._width.value/@frame._height.value) | |||
There was a problem hiding this comment.
@mattpap Is there a better way to do this / communicate an optional ratio to the subsequent calls to update below?
|
With this change the original issue code looks as expected:
|
| @@ -216,43 +216,22 @@ export class PlotCanvasView extends DOMView | |||
| follow_enabled = false | |||
| has_bounds = false | |||
|
|
|||
| r = null | |||
There was a problem hiding this comment.
@mattpap is there a better way to subsequently communicate this optional value to update ?
There was a problem hiding this comment.
No, this is idiomatic javascript.
| this._plot_bounds[bounds_id] = this._compute_plot_bounds(renderers, bounds) | ||
| let total_bounds = this._compute_plot_bounds(renderers, bounds) | ||
|
|
||
| if (r != null) |
There was a problem hiding this comment.
This should have resulted in an error, because r is a number, not number | null (or similar), but apparently it compiles. Looks like another bug in the compiler. Anyway, given r optional nature, the signature of this method should be changed to r?: number. I would also change r to something more meaningful (ratio, aspect_ratio).
| @@ -93,6 +93,32 @@ export class DataRange1d extends DataRange { | |||
| return result | |||
| } | |||
|
|
|||
| adjust_bounds_for_aspect(bounds: Rect, r: number) : Rect { | |||
| let result = bbox.empty() | |||
| @@ -93,6 +93,32 @@ export class DataRange1d extends DataRange { | |||
| return result | |||
| } | |||
|
|
|||
| adjust_bounds_for_aspect(bounds: Rect, r: number) : Rect { | |||
The commit moves the aspect ratio adjust (for a single plot update) to after the unioned bounds are computed for the DataRange rendereres. Previously the aspect correction happened on all the individual renderer bounds, before the call to update. This could result in incorrect aspect-corrected overall bounds in some situations. Note: use of match_aspect with linked DataRange1d across multiple plots will probably yield undefined/undesirable behaviour and should be considered unsupported.
bryevdv
force-pushed
the
bryanv/7218_circles
branch
from
cf79cb7
to
9e7570b
Compare
The commit moves the aspect ratio adjust (for a single plot update) to after the unioned bounds are computed for the DataRange renderers. Previously the aspect correction happened on all the individual renderer bounds, before the call to update. This could result in incorrect aspect-corrected overall bounds in some situations.
Note: use of match_aspect with linked DataRange1d across multiple plots will probably yield undefined/undesirable behavior and should be considered unsupported.