Skip to content

Line should use the projection by default, if available #1165

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

Closed
Fil opened this issue Dec 7, 2022 · 6 comments · Fixed by #1171
Closed

Line should use the projection by default, if available #1165

Fil opened this issue Dec 7, 2022 · 6 comments · Fixed by #1171
Labels
enhancement New feature or request geo Maps and projections question Further information is needed

Comments

@Fil
Copy link
Contributor

Fil commented Dec 7, 2022

The way we implemented Plot.line with a projection had us skip the x and y scales if the curve was "projected" (see https://github.com/observablehq/plot/pull/1156/files#r1041916802). This works in practice, but the underlying reason for this branching is that the projected curve needs the raw (unprojected) coordinates, not that it wants to hides the channels from the scales (which in this case is the projection).

This need seems generic enough that we should try and consider systematically passing both the scaled and the unscaled channels to render(); so marks that need the unscaled values could have them without hiding these channels from the scale initializers?

In the particular case of Plot.line, skipping x and y is not a problem, since the projection doesn't set its domain from the channels; one might even argue that the current code is faster, since with this proposal we would compute projected coordinates "for nothing". But still I find this branching inelegant, and it doesn't allow us to set curve=projected as a default for when the mark is used with a projection—which would be my preference.

@Fil Fil added enhancement New feature or request question Further information is needed geo Maps and projections labels Dec 7, 2022
@mbostock
Copy link
Member

mbostock commented Dec 7, 2022

I considered passing the unsealed channels previously and deliberately avoided it because I feel like it adds further complexity to the mark.render interface.

I think we could equivalently change the meaning of curve: "linear" when there is a projection, rather than requiring curve: "projected". (I’m not sure we need both? Though maybe there is some edge case.)

The goal here is just to fix the “inelegant branching”? Or is there some new user-visible functionality that this would unlock?

@Fil
Copy link
Contributor Author

Fil commented Dec 7, 2022

The functionality is to have curve transparently default to "projected".

My thinking is that since the mark constructor can't access the options, there is no way at this stage to determine (implicitly) if there will be a projection in a plot that uses it. It's only decidable in render(), when we can read the context.

Also, we should keep "linear": for example a notebook that would teach the difference between planar and great circle routes would need to use both (like I did for the demo of planar vs spherical voronoi).

@mbostock
Copy link
Member

mbostock commented Dec 7, 2022

So, defaulting to curve: "auto" that automatically chooses between linear and projected at render would do it?

@Fil
Copy link
Contributor Author

Fil commented Dec 7, 2022

Yes, exactly. What I didn't figure in this case was how to implement it so that:

  • the scales are computed correctly in the general case (so, passing scale:"x" in the constructor)
  • when render needs unscaled values (because projection), it has access to them

@mbostock mbostock changed the title Pass unprojected/unscaled values to render? Line should use the projection by default, if available Dec 7, 2022
@mbostock
Copy link
Member

mbostock commented Dec 7, 2022

I’ve retitled this issue so that it tries to capture the desired user functionality.

I don’t see a way of doing this currently because, as you say, we need to know in the constructor whether the x and y channels should be scaled (projected) or whether the mark will handle projection during render. And even if we found a way to use the projection during render on the unscaled values, presumably we’d still have pass the x and y channels through the projection and then ignore the result, which is wasted effort.

Do think this is urgent or a blocker? It doesn’t feel like a lot of work to add curve: "projected" to a Plot.line when you need it (or to use Plot.geo with a LineString), though it would certainly be nice if it were automatic.

@Fil
Copy link
Contributor Author

Fil commented Dec 7, 2022

I don't think it's a blocker, but it will be a breaking change when we change it, for people who expected the default to be linear when they hadn't specified a curve. But, for the best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request geo Maps and projections question Further information is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants