Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ascent and descent calculation in panorama #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lexi-lambda
Copy link
Member

Fixes #63.

@rfindler
Copy link
Member

rfindler commented Jun 9, 2020

This one seems, a priori, worrying from a backwards compatibility perspective. But since it only affects the descent in the result of panorama then, at least in the way I tend to use that function (I feel like I don't use baseline appending or superimposition on panorama'd picts), maybe this is possible.

(I'll try running some old talks and see if things look broken and report back. I tried running some old talks and I don't anything that looks broken.)

@lexi-lambda
Copy link
Member Author

Thanks for checking! I agree that this is a much more significant change than my other recent PRs.

My rationale for just making this change was that this is very clearly a bug: the existing behavior is both strange and somewhat unpredictable. Intentionally depending on the current behavior seems very difficult, since I couldn’t even understand what it was doing until I carefully read the implementation. On the other hand, it wouldn’t surprise me if someone somewhere was depending on it unintentionally.

But what’s the alternative, really? A new #:correct-baseline? keyword option? A parameter? pict-2? I’m not sure what I’d do instead.

@rfindler
Copy link
Member

rfindler commented Jun 10, 2020

I understand your rationale. I don't think the "it is a bug" rationale is sufficient. "It is a bug and no one seems to depend on it" is more to my taste for a rationale that passes muster. My best hope is that that's what happens and maybe others will contribute. Maybe you can too if you like, by finding code and trying things out (not that you have to, of course).

As for alternatives, I would point to the #:draw-border? argument to filled-rectangle which, it seems to me, is exactly analogous. It papers over a bug and I have memorized that argument because I use it routinely and I have never been in a situation where I wanted the default value (only in situations where the default value didn't break things enough that I cared).

@lexi-lambda
Copy link
Member Author

As for alternatives, I would point to the #:draw-border? argument to filled-rectangle?

I agree that the default there is rarely what I want, but at least in that case there are some legitimate uses. I am sympathetic to your broader point, though.

I will admit it is unlikely that I will take the time to actively audit other people’s code to see how this affects things, but I also don’t feel any particular urgency to get this fixed. I can just run off my fork for now, so I’m happy to wait and let other people weigh in. If this fix is ultimately deemed too incompatible, so be it.

@rfindler
Copy link
Member

My experiments are positive which, in addition to pointing out that there are uses of panorama where this change doesn't matter (just like with filled-rectangle), is good, but I would feel better if there were more.

@mflatt
Copy link
Member

mflatt commented Jun 10, 2020

I have few uses of panorama, but the change is ok for those as far as I see.

@lexi-lambda lexi-lambda changed the title Fix descent calculation in panorama Fix ascent and descent calculation in panorama Jun 11, 2020
@lexi-lambda
Copy link
Member Author

I realized today that the ascent calculation is actually also incorrect, it just shows up in different situations. So I’ve now pushed a change that fixes that, too.

@rfindler
Copy link
Member

rfindler commented Jul 2, 2020

Should there be two test cases?

@rfindler
Copy link
Member

It seems safe to merge this. Any objections?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panorma calculates the descent incorrectly
3 participants