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

Define default for layers + decide to use radians or degrees #73

Closed
cabanier opened this issue Apr 14, 2020 · 16 comments
Closed

Define default for layers + decide to use radians or degrees #73

cabanier opened this issue Apr 14, 2020 · 16 comments

Comments

@cabanier
Copy link
Member

/agenda Should centralAngle be in degrees or radians? See also here

@probot-label probot-label bot added the agenda label Apr 14, 2020
@cabanier
Copy link
Member Author

/agenda what are good defaults for all the layers?

@cabanier cabanier changed the title Should centralAngle be in degrees or radians? Define default for layers + decide to use radians or degrees Apr 14, 2020
@cabanier
Copy link
Member Author

cabanier commented Apr 14, 2020

During today's call I heard from @ada, @Manishearth and @toji that they prefer radians.
I still think that degrees is easier to use, for example:
degrees:

let layer = xrGLFactory.createCylinderLayer(texture", {pixelWidth: 200, pixelHeight: 100});
layer.centralAngle = 45;

radians:

let layer = xrGLFactory.createCylinderLayer(texture", {pixelWidth: 200, pixelHeight: 100});
layer.centralAngle = Math.PI / 4;

We still need to discuss the defaults so maybe we can go over it during that meeting. If noone else agrees with me, I will make the change.

@toji
Copy link
Member

toji commented Apr 14, 2020

Worth noting that this particular topic felt like it got derailed a bit early, so the radians majority above may not be a complete picture. I do think it's a good topic to discuss along with the defaults, though.

@Manishearth
Copy link
Contributor

I will note (and I think @AdaRoseCannon mentioned this), APIs tend to have degrees in the variable name when they use degrees. But this isn't all of them, as you mentioned DOMMatrix doesn't (nor can it, it's a constructor). But if we pick degrees we can just call the field centralAngleDegrees. It's a bit cumbersome, though.

@cabanier
Copy link
Member Author

Alternatively, we can specify that distance and angle are represented by CSS units.
So:

layer.centralAngle = "45deg";
layer.radius = "150cm";

@Manishearth
Copy link
Contributor

I don't know of any other API that does that, though it's an interesting idea. Remember that full CSS supports calc and variables and all kinds of nonsesnse in that context, and we probably don't want that. I'm not sure if the spec gives us the ability to distinguish, this is all parse time so if we want to refer to css' parsing algorithms we need a parsing context which we don't have.

@cabanier
Copy link
Member Author

cabanier commented Apr 14, 2020

The DOMMatrix constructor uses it. I've seen other APIs but I can't find them at the moment.

Just looking through github for CSS that uses angles, 15 pages in there was not a single example of using radians.

@Manishearth
Copy link
Contributor

The DOMMatrix constructor uses it

Right, it uses degrees directly, not 1234deg, which is what i was talking about.

If you want examples of functions using radians: all of Math is radian based, as are all of the CSS trig functions. It's certainly not as clear cut as I originally thought it was 😄

@cabanier
Copy link
Member Author

The DOMMatrix constructor uses it

Right, it uses degrees directly, not 1234deg, which is what i was talking about.

No. Look at the constructor. It takes a string that is parsed down with CSS rules.

@Manishearth
Copy link
Contributor

Oh, I forgot about that constructor.

It seems underspecified, it doesn't say how to handle percentages, calc, and things like em units (this is precisely what i want to avoi having to deal with here)

@cabanier
Copy link
Member Author

It seems underspecified, it doesn't say how to handle percentages, calc, and things like em units (this is precisely what i want to avoi having to deal with here)

Section 2 of Parsing a string into an abstract matrix covers that.

@cabanier
Copy link
Member Author

So choices are:

  1. use radians which is easier to do calculations on but is not preferred by develops
  2. use degrees which is more difficult to calculate but easy to use
  3. use a CSS value which is the most flexible but the hardest to specify

If we go with option 3, all distance units should probably also become CSS values.

@Manishearth
Copy link
Contributor

All distance units definitely should not become CSS values, CSS can only specify things in screen space, this is world space where we have a good established convention of using meters. CSS doesn't do "real units" that well, since it depends on the anchoring.

I'd dispute the "preferred by developers" in the general because radians are more natural when you get them out of other calculations.

However, for cylinder layers I would expect the number to be hardcoded, in which case degrees makes a little bit more sense as being preferred by the devs. But in that case i slightly feel like we should have "degrees" in the variable name to be clear.

@tabatkins
Copy link

If the API takes a JS Number value, it should definitely be radians, to match with all the Math apis that consume or return angles.

(Unless, as Manish said, the fact that it takes degrees is clearly specified in the name; I suppose that would be okay.)

Taking a string that's a CSS value is... possible, but yeah, would be weird. Note that it's definitely possible to restrict it to just unit values, no calcs; that doesn't fall out of the simplest way to define the parsing, but it's straightforward to do so. Something like

[=CSS/parse=] the string as an <angle>, and let |result| be the result. If |result| is failure, or anything but a [=dimension=], (do error stuff). Otherwise, convert |result| to the deg unit, and let |angle| be the |result|'s numeric value.

@toji
Copy link
Member

toji commented Apr 21, 2020

Asked on the call and radians was a pretty overwhelming favorite. Rik said he'll make the spec change soon.

@cabanier
Copy link
Member Author

Asked on the call and radians was a pretty overwhelming favorite. Rik said he'll make the spec change soon.

Fixed in PR #92

@cabanier cabanier removed the agenda label Apr 21, 2020
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

No branches or pull requests

4 participants