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

Vector types: vec2, vec3, vec4 #166

Merged
merged 4 commits into from
Jan 16, 2017
Merged

Vector types: vec2, vec3, vec4 #166

merged 4 commits into from
Jan 16, 2017

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jan 9, 2017

For #2

#138 will build off this.

Cesium implementation: CesiumGS/cesium#4759

To Do:

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 10, 2017

[ ] Should it be possible to do component-wise operations between Color and vec4?

What does this mean add(color, vector)?

[ ] Similarly can a style that evaluates color be set to a vec4? In the Cesium GLSL implementation this automatically works because both types are represented by vec4. E.g. color : vec4(1.0)
[ ] If we don't go with the above two, we should at least support casting between colors and vectors. Like color(vec4(1.0)) and vec4(rgba(0,0,0,1))

Do we need a Color type at all? Is it reasonable to implement it as vec4 and be just as easy to use?

If not, in the GLSL world, these would be separate types so I suppose they should not have implicit conversion and instead should require explicit construction (color(vec4(1.0)) and vec4(rgba(0,0,0,1))). Is that hard to implement?

* `vec4(vec3, Number)` - initialize with a `vec3` and number
* `vec4(Number, vec3)` - initialize with a `vec3` and number

`vec2` components may be accessed with `.x`, `.y` and `[0]`, `[1]`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is in the vec4 section. Either create a new section here or move some of these sentences to be in the right vector type section just above.


Vectors support the following binary operators by performing component-wise operations: `===`, `==`, `!==`, `!=`, `+`, `-`, `*`, `/`, and `%`. For example `vec4(1.0) === vec4(1.0)` is true since the x, y, z, and w components are equal. This is not the same behavior as a JavaScript `Object`, where, for example, reference equality would be used. Operators are essentially overloaded for `vec2`, `vec3`, and `vec4`.

Operations between vectors of different types will not evaluate component-wise, but instead will evaluate as JavaScript objects. For example `vec2(1.0) === vec3(1.0)` is false and `vec2(1.0) * vec4(1.0)` is `NaN`. See the [Conversions](#conversions) section for more details.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkward. Can we do better?

vec2(1.0) === vec3(1.0)

What does this do in GLSL? Based on the constructor overloads, is one converted to the other?

Instead of returning false/NaN for these examples, is it reasonable to implement if these are type mismatch errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GLSL will not compile with that, you need to cast explicitly.

Equality checks could be made to work without too much trouble, the multiply would be harder (is the result vec2 or vec4?). Originally all this would work if vec2, vec3, and vec4 were treated as the same vec4 type, but I thought the point of moving away from that was to prevent expressions like these from working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, is it reasonable to implement if these are type mismatch errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I misunderstood your comment.

It shouldn't be hard to detect type mismatch errors in the JS backend, as usual it's more difficult in the GLSL backend, but still possible to just catch a shader failure and throw a generic type mismatch error (as talked about offline).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, let's go with the type mismatch.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 10, 2017

Just those comments.

@lilleyse
Copy link
Contributor Author

What does this mean add(color, vector)?

Expressions like color() + vec4(1.0) or color() * vec4(1, 0, 0, 1)

If not, in the GLSL world, these would be separate types so I suppose they should not have implicit conversion and instead should require explicit construction (color(vec4(1.0)) and vec4(rgba(0,0,0,1))). Is that hard to implement?

This shouldn't be too hard to implement.

Do we need a Color type at all? Is it reasonable to implement it as vec4 and be just as easy to use?

I think its nice to have a color type in case we want to add special functions on colors. At the moment though its nearly the same as a vec4.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 10, 2017

I think the right thing to do is to use vec4 for colors. This is not a huge spec update. How big is the code update?

If we need color-specific functions, they would just be stand alone functions, not member functions.

GLSL is a pretty good example of getting good milage out of using vec4 for colors. You could also evaluate the Cesium color and Cartesian types to see if this is reasonable.

As simple as possible, but no more simple.

@lilleyse
Copy link
Contributor Author

I think the right thing to do is to use vec4 for colors. This is not a huge spec update. How big is the code update?

The code update is not too bad at all. This approach sounds good with me too, more flexible.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 11, 2017

I think the right thing to do is to use vec4 for colors. This is not a huge spec update. How big is the code update?

The code update is not too bad at all. This approach sounds good with me too, more flexible.

Great, let's do it!

@lilleyse
Copy link
Contributor Author

I opened a PR for handling colors as vec4. CesiumGS/cesium#4849

I'll also have a PR for checking type mismatching.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 14, 2017

@lilleyse given CesiumGS/cesium#4867, is this ready to finish?

@lilleyse
Copy link
Contributor Author

This is ready for another look. The new changes describe colors in terms of vectors.

There will be a separate PR for type mismatching since that affects pretty much everything including vectors.

Need to update the built-in functions with vector support. Currently the built-in functions are on https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/math-functions which will be merged relatively soon. There are just a few more unary operations remaining.

I'm going to open a PR soon for the built-in math functions, and in that PR it will support vector types.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 16, 2017

Looks great!

@pjcozzi pjcozzi merged commit 25fafcd into master Jan 16, 2017
@pjcozzi pjcozzi deleted the vector branch January 16, 2017 17:01
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.

2 participants