More unary fns for 3d-tiles #4775

leesafini · 2016-12-20T20:22:03Z

For #4604

Implements within 3D-Tiles:

sign(x)
floor(x)
ceil(x)
round(x)
exp(x)
log(x)
exp2(x)
log2(x)
fract(x)

A few notes:

fract has been added to CesiumMath. According to the Wikipedia page for fractional parts, there are a couple of ways of handling negative inputs. I've done the basic, where fract(x) when x < 0 outputs x - floor(x). This means that the output of fract(-2.25) would be 0.75. If there's another way you would prefer this to be handled, please let me know.
Apparently JavaScript doesn't have a build-in function for exp2 either, so I've added this to CesiumMath as well
I keep getting shader failures for round. Still working on this, but let me know if you can see any glaring problems.

…iles' into unaryfns

…ryfns

vertex shader for round still won't compile - not sure why

JavaScript apparently doesn't have exp2 function?

leesafini · 2016-12-20T20:47:03Z

@lilleyse This is telling me I have conflicts in ExpressionSpec, but it seems fine when I look through it myself. How can I tell where the conflict is?

lilleyse · 2016-12-21T14:20:17Z

I see some merge conflicts when merging the latest 3d-tiles into this branch. They also appear if you click the Resolve conflicts button. Though it looks like most of the conflicts are pretty simple to fix.

lilleyse

Looks great! The tests are really solid.

The only thing is I'm not sure if exp2 and fract belong in CesiumMath, but I'm open to other opinions.

lilleyse · 2016-12-21T14:21:41Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

+addStyle('Sign', {
+    color : "color() * sign(${temperature})",
+    pointSize : "5"
+});


Since temperature is always positive this style won't look very special. One idea could be using the sign of the ${POSITIONS} values (like the tests below).

Just a reminder about this comment. fract sort of applies here too. Even just sign(${temperature} - 0.5) may be sufficient.

lilleyse · 2016-12-21T14:27:53Z

Source/Scene/Expression.js

+        sign : Math.sign,
+        floor : Math.floor,
+        ceil : Math.ceil,
+        round : Math.round,


It turns out that round isn't supported in GLSL ES 1.0.

In Node.prototype.getShaderExpression you will need to add a special case for it - you can substitute round with floor(left + 0.5).

I'm having a lot of trouble with this. Currently getting Vertex shader failed to compile. Compile log: ERROR: 0:8: '0.5' : syntax error but it's saying the error is in files I'm pretty sure I've never modified.

Not really sure if I'm approaching this in the right way, anyway - I added in the special case for round and had to remove it from the unary functions, and in doing so had to add in a case for it in parseCall like I had done before the data-driven approach was implemented.

I think round can be treated like the other unary fns in that section. Only the shader area needs to have the special case.

lilleyse · 2016-12-21T14:29:24Z

Source/Scene/Expression.js

+        ceil : Math.ceil,
+        round : Math.round,
+        exp : Math.exp,
+        exp2 : CesiumMath.exp2,


I don't think exp2 is essential to add to CesiumMath. Instead create a function in Expression.js for exp2 and reference that in the unaryFunctions list.

Similarly I would do the same thing for fract.

@pjcozzi What do you think here?

Agreed, #4775 (comment)

lilleyse · 2016-12-21T14:42:08Z

Source/Scene/Expression.js

+        exp2 : CesiumMath.exp2,
+        log : Math.log,
+        log2 : Math.log2,
+        fract : CesiumMath.fract


I've done the basic, where fract(x) when x < 0 outputs x - floor(x). This means that the output of fract(-2.25) would be 0.75. If there's another way you would prefer this to be handled, please let me know.

This is the way GLSL does it, so that's good.

pjcozzi · 2017-01-02T15:39:44Z

The only thing is I'm not sure if exp2 and fract belong in CesiumMath, but I'm open to other opinions.

Perhaps just keep them local and not expose them to keep the surface area of the Cesium API down.

lilleyse · 2017-01-09T15:27:17Z

@leesafini is this ready to review? #4759 was merged recently so there are a few merge conflicts.

leesafini · 2017-01-09T17:28:25Z

@lilleyse Sorry, not ready yet! Having issues with how to call fract and exp2 from within Expression. Today's my birthday, so I've had a slow weekend with progress.

lilleyse · 2017-01-09T17:53:51Z

Ok thanks for the heads up, and happy birthday!

lilleyse · 2017-01-12T16:35:24Z

Source/Scene/Expression.js

@@ -1184,6 +1223,8 @@ define([
                    return 'bool(' + left + ')';
                } else if (value === 'Number') {
                    return 'float(' + left + ')';
+                } else if (value === 'round') {
+                	return 'floor(' + left + '0.5)';


Try 'floor(' + left + ' + 0.5)';

Hahaha oof, that fixed it. Sorry!

leesafini · 2017-01-12T16:37:11Z

@lilleyse pretty sure this is ready to view, but the conflicts it's telling me need to be resolved are just the changes I've made in this branch...

leesafini · 2017-01-12T17:26:37Z

@lilleyse Made the changes you suggested for round, now definitely ready to view

lilleyse

I fixed the merge conflict - it was definitely a weird one...

Besides my comments, there are also a couple test failures.

lilleyse · 2017-01-12T18:29:54Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

+addStyle('Sign', {
+    color : "color() * sign(${temperature})",
+    pointSize : "5"
+});


Just a reminder about this comment. fract sort of applies here too. Even just sign(${temperature} - 0.5) may be sufficient.

lilleyse · 2017-01-12T18:33:50Z

Source/Scene/Expression.js

@@ -104,7 +104,16 @@ define([
        asin : Math.asin,
        atan : Math.atan,
        radians : CesiumMath.toRadians,
-        degrees : CesiumMath.toDegrees
+        degrees : CesiumMath.toDegrees,
+        sign : Math.sign,


Use CesiumMath.sign instead. Looking here Math.sign isn't supported in all browsers: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/sign.

lilleyse · 2017-01-12T18:34:13Z

Source/Scene/Expression.js

@@ -1404,6 +1413,14 @@ define([
        return expressions;
    }

+    function fract(number) {


Place these functions closer to the top, maybe right before var binaryFunctions = {.

lilleyse · 2017-01-12T18:37:23Z

Source/Scene/Expression.js

+        exp : Math.exp,
+        exp2 : exp2,
+        log : Math.log,
+        log2 : Math.log2,


It turns out this is also not supported for IE. log2 will also need to be a local function which can use CesiumMath.logBase.

leesafini · 2017-01-13T03:48:08Z

@lilleyse Good to review.

lilleyse · 2017-01-13T14:37:17Z

Awesome, thanks for finishing this up @leesafini.

lilleyse · 2017-01-13T14:38:32Z

Also don't worry about the spec changes, I'll add those.

leesafini added 19 commits December 19, 2016 11:41

Merge remote-tracking branch 'refs/remotes/AnalyticalGraphicsInc/3d-t…

ffd91d1

…iles' into unaryfns

Merge remote-tracking branch 'refs/remotes/origin/binaryfns' into una…

3592d81

…ryfns

begin adding unary fns

888a454

added fract to CesiumMath library

d9fbc91

added tests for fract to MathSpec

a3be0fb

added tests in expressionspec

2d3dd6e

added tests for fract to expressionspec

8d7e29d

added shader tests

9591c46

added sandcastle demos

ba682da

some updates

d2ac9f3

vertex shader for round still won't compile - not sure why

changed exp2 to CesiumMath

cd6b804

JavaScript apparently doesn't have exp2 function?

some updates

664bf5c

fixed more conflicts

bfe278c

fixed whitespace

e9fd5e1

fixed more conflicts

59e2209

more conflicts

883dc4d

package newline?

e6e37ac

final conflicts maybe

7210fbd

last changes to remove extra files

7e390ea

leesafini changed the title ~~Unaryfns~~ More unary fns for 3d-tiles Dec 20, 2016

lilleyse reviewed Dec 21, 2016

View reviewed changes

leesafini added 2 commits January 6, 2017 14:11

moving fract and exp2 from CesiumMath to Expression

8cb17a6

removed CesiumMath tests

0c1313e

pjcozzi added the priority - high label Jan 11, 2017

leesafini added 3 commits January 12, 2017 11:17

trying to fix round

c2eb1ec

moved exp2 and fract to Expression

0635a06

resolving merge conflicts

7e85fd4

lilleyse reviewed Jan 12, 2017

View reviewed changes

round working

1bf372e

changed round case to only shader area

034b4f1

lilleyse added 2 commits January 12, 2017 13:15

Merge branch '3d-tiles' into unaryfns

6945d48

Fix up merge

eaaa724

lilleyse force-pushed the unaryfns branch from d8c6397 to eaaa724 Compare January 12, 2017 18:28

lilleyse reviewed Jan 12, 2017

View reviewed changes

leesafini added 4 commits January 12, 2017 20:35

changes made per reviewed

af2704f

fixed tests

9e8ee76

round test

7650e3d

more round testing

792d246

lilleyse merged commit 69fcd93 into CesiumGS:3d-tiles Jan 13, 2017

lilleyse mentioned this pull request Jan 13, 2017

More unary math functions CesiumGS/3d-tiles#167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More unary fns for 3d-tiles #4775

More unary fns for 3d-tiles #4775

leesafini commented Dec 20, 2016

leesafini commented Dec 20, 2016

lilleyse commented Dec 21, 2016

lilleyse left a comment

lilleyse Dec 21, 2016

lilleyse Jan 12, 2017 •

edited

Loading

lilleyse Dec 21, 2016

leesafini Jan 12, 2017

lilleyse Jan 12, 2017

lilleyse Dec 21, 2016

lilleyse Dec 21, 2016

lilleyse Dec 21, 2016

pjcozzi Jan 2, 2017

lilleyse Dec 21, 2016

pjcozzi commented Jan 2, 2017

lilleyse commented Jan 9, 2017 •

edited

Loading

leesafini commented Jan 9, 2017

lilleyse commented Jan 9, 2017

lilleyse Jan 12, 2017

leesafini Jan 12, 2017

leesafini commented Jan 12, 2017

leesafini commented Jan 12, 2017

lilleyse left a comment

lilleyse Jan 12, 2017 •

edited

Loading

lilleyse Jan 12, 2017

lilleyse Jan 12, 2017

lilleyse Jan 12, 2017

leesafini commented Jan 13, 2017

lilleyse commented Jan 13, 2017

lilleyse commented Jan 13, 2017

More unary fns for 3d-tiles #4775

More unary fns for 3d-tiles #4775

Conversation

leesafini commented Dec 20, 2016

leesafini commented Dec 20, 2016

lilleyse commented Dec 21, 2016

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 2, 2017

lilleyse commented Jan 9, 2017 • edited Loading

leesafini commented Jan 9, 2017

lilleyse commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesafini commented Jan 12, 2017

leesafini commented Jan 12, 2017

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesafini commented Jan 13, 2017

lilleyse commented Jan 13, 2017

lilleyse commented Jan 13, 2017

lilleyse Jan 12, 2017 •

edited

Loading

lilleyse commented Jan 9, 2017 •

edited

Loading

lilleyse Jan 12, 2017 •

edited

Loading