rmaia / pavo

tools for the analysis of color data in R
http://pavo.colrverse.com
GNU General Public License v2.0
68 stars 17 forks source link

Custom-margin-resetting causing issues when adding points to some plots #219

Closed thomased closed 3 years ago

thomased commented 3 years ago

The margin-reset-on-exit changes cran wanted as part of 2.6.1 have created a little issue when adding points to plots that require custom margins. E.g. the following should add a point at the origin, but it's offset owing to margin differences (I think?).

data(flowers)
vis.flowers <- vismodel(flowers,
                        visual = "apis", qcatch = "Ei", relative = FALSE,
                        vonkries = TRUE, achromatic = "l", bkg = "green"
)
hex.flowers <- colspace(vis.flowers, space = "hexagon")
plot(hex.flowers)
points(x = 0, y = 0, col = 'red', pch = 20)
Screen Shot 2021-01-14 at 9 25 05 pm

Not sure what the simplest solution would be yet.

(As an aside, some reliable way to test images would be lovely...)

Bisaloo commented 3 years ago

The best solution IMO would be to stop messing with margins. Users can set par() themselves if they wish. But I'm not even convinced it's necessary in most cases.

thomased commented 3 years ago

Hah, well that's a fair point. I'll scroll through them all & check but you might well be right.

I'm not sure if we needed to tweak the 3d plot(s) for good reason, but perhaps not...

Bisaloo commented 3 years ago

I think the current code calls for margin tweaking in some cases but my sentiment is that it's a suboptimal behaviour and we should transform this code to avoid it rather than treating the symptom and tweaking the margins

Bisaloo commented 3 years ago

On a related note, par(pty = "s") should be removed to use plot(asp = 1) (which seems closer to the desired effect anyways).

thomased commented 3 years ago

Ah, cheers. Will do.

So after a quick look we can remove all the margin-fiddling for the 2d-plots without issue. I guess we need to do the proper deprecation procedure? Or can we just ditch it since I doubt many people really use it anyway...

thomased commented 3 years ago

Fixed in #220 — thanks.