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

3D Tiles - ternary functions #4694

Closed
lilleyse opened this issue Nov 30, 2016 · 5 comments
Closed

3D Tiles - ternary functions #4694

lilleyse opened this issue Nov 30, 2016 · 5 comments

Comments

@lilleyse
Copy link
Contributor

lilleyse commented Nov 30, 2016

@Dylan-Brown

Before getting into vector math, there are two more functions to handle. You'll need to add TERNARY to ExpressionNodeType.js. Handling three operators will be similar to here: #4688 except you'll use left, right, and test. The x values below can be passed as the test argument for new Node.

clamp(x, minVal, maxVal) - use CesiumMath.clamp
mix(minVal, maxVal, x) - use CesiumMath.lerp

Even though this is just two functions, use the data-driven approach like before so it is easy to add others.

@Dylan-Brown
Copy link
Contributor

Dylan-Brown commented Dec 1, 2016

@lilleyse
Sounds good! I'll get working on it.
Also, I noticed in Math.js, where CesiumMath is defined, the grammar in the comment above the definition of clamp is wrong. It says "Constraint a value" where it should be "Constraints a value." Should I fix this in my PR, or should I do it separately in a PR to master, or what?
Also, should I call the function lerp or should I call it mix?

@Dylan-Brown
Copy link
Contributor

Dylan-Brown commented Dec 1, 2016

Also, here's something else:

CesiumMath.clamp = function(value, min, max)
CesiumMath.lerp = function(p, q, time)

Applying left, right, and test to them using a data driven approach doesn't make sense, since I'm assuming

  • for clamp, left=min, right=max, and test=value
  • for lerp, left=p, right=q, test = time

and to use a data driven approach, I would write something like

if (defined(ternaryFunctions[value])) {
    return value + '(' + left + ', ' + right + ', ' + test + ')';
}

and so the definitions of the functions, specifically, the ordering of the arguments, doesn't quite work out

@lilleyse
Copy link
Contributor Author

lilleyse commented Dec 1, 2016

Should I fix this in my PR, or should I do it separately in a PR to master, or what?

You can do it in this PR

Also, should I call the function lerp or should I call it mix?

We tend to side with the glsl naming, so go with mix.

and so the definitions of the functions, specifically, the ordering of the arguments, doesn't quite work out

Yeah true... I'd be ok if the 1st and 2nd params go to left and right while the 3rd goes to test, regardless of the meaning of the parameters.

@Dylan-Brown
Copy link
Contributor

@lilleyse
I believe this is done, unless there's other work for the ternary functions!

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 14, 2017

@lilleyse OK to close this?

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

No branches or pull requests

3 participants