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

Put some symbols behind the y_down feature flag #353

Closed
wants to merge 1 commit into from

Conversation

@nical
Copy link
Collaborator

nical commented Jul 2, 2019

Some non-gecko/servo people have complained to me about euclid dictating that y points downward through a few names like Rect::top, the side offsets code (etc.), because their app/game/crate uses the y-up convention or is agnostic to these.

At the same time having names like top() is very convenient in our own code, so a simple compromise is to opt into it explicitly using a feature flag which we'll use in the servo and gecko ecosystems but others can just ignore.
We can also add an y_up feature flag that adds these symbols for the other convention, but i'd rather add it when someone asks for it otherwise it'll just be dead code. The main advantage is that asking for both y_up and y_down would generate errors at compile time which seems desirable.

There's a discussion about this in #317 in which we talk about finer granularity (for example per unit or per file). It would have been really nice to have it per unit but it's hard to do it in rust without making things complicated (specialization would have helped a lot).

So I think that the feature flag approach is both simple and good enough to address the original complaints and our needs.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 2, 2019

r? @kvark

@kvark
Copy link
Member

kvark commented Jul 2, 2019

It's not clear to me that there is enough weight for the issue to add a new feature flag. They are always a pain - difficult to test (surprises on CI runs), difficult to combine sometimes, and fragment the ecosystem (i.e. what if one of your dependencies decides that it needs this feature, while you do not?).

I would prefer this to be solved by renaming the methods to truly become direction agnostic, i.e. x0y0(), x1y0, x1y1(), and x0y1() or a similar scheme.

@nical
Copy link
Collaborator Author

nical commented Jul 2, 2019

This is strictly additive, so if you do not use the flag it means you are agnostic to the meaning of up and down and whether or not another dependency isn't agnostic there should be no problem (no more than today anyway).
I think that the fragmentation issue is made worse by strictly enforcing a meaning for the direction of the y-axis.

I worry that names like x0y0() bring a lot more cognitive overhead than top_left(). for rectangles we could probably get away with min/max/min_x/max_x/etc. I'm not sure how we could dodge these names with side offsets.

If we are to change the naming scheme, I'd rather make sure servo layout folks sign off on it because they'll be more impacted than WebRender.
The y_down feature flag's main appeal is that it causes minimum churn. We just have to enable a feature flag on a bunch of servo crates and continue like we did before, and at the same time resolve the problem that's been annoying people outside the servo ecosystem.

@SimonSapin @Manishearth thoughts?

@Manishearth
Copy link
Member

Manishearth commented Jul 2, 2019

I want to avoid exposing both conventions, but putting the existing stuff behind a feature flag makes a lot of sense to me.

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.