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

Grid material and Ellipsoid primitive #601

Merged
merged 33 commits into from
Apr 3, 2013
Merged

Grid material and Ellipsoid primitive #601

merged 33 commits into from
Apr 3, 2013

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Mar 27, 2013

This is my first attempt at creating a Cesium material.

This branch now includes another merged branch with changes to the EllipsoidPrimative to make it appear double-sided (to show the far wall of the ellipsoid even when outside).

There are some known depth issues where ellipsoids intersect, but these existed prior to this branch and may eventually be addressed by #607.

This branch used to include logic for adding additional grid lines. This code has been reverted, as it made overlapping ellipsoids look too crowded. It is still available for inspection on other branches.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 28, 2013

Cool. I won't be able to check this out until next week, but @bagnell is welcome to review and merge this sooner if you want it to make b15. At a quick glance, the code looks OK.

Also, update CHANGES.md and the Fabric Wiki.

@emackey
Copy link
Contributor Author

emackey commented Mar 28, 2013

No rush. Before this goes in I could use some advice on how to get the Volume demo to show both the front and back sides of the ellipsoids. Currently when the camera is outside an EllipsoidPrimitive, it only sees the outside wall (facing the camera), but when the camera flies inside, it sees the inside wall. It's as if the backface culling changes when the camera goes inside, or maybe the depth buffer is blocking the farther wall. Is there any way to fix this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 28, 2013

In some cases, this will actually be useful. We can also imagine different materials for the front and back. For example, the front has thick lines and the back has thin lines. I coded all of this in Insight3D, but with the fixed function. It is much easier with GPU ray casting.

Anyway, the issue is we only take the closest ray intersection, not both. So if the closest one has an alpha of zero, we never check the back. We could do both and even refraction if we want. This will require a change to the ellipsoid itself, which we should do (at least initially) in a separate pull request.

uniform vec4 gridColor;
uniform float holeAlpha;
uniform vec2 lineCount;
uniform vec2 lineThickness;
Copy link
Contributor

Choose a reason for hiding this comment

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

What unit is lineThickness in? I want them to be pixels and for the thickness to be uniform in screenspace, just like polylines are. That doesn't look like what's happening now though. Also, why call it lineThickness when we use width everywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The units are different depending whether your GPU supports GL_OES_standard_derivatives. Without derivatives, the unit is a percentage, where 0.0 is no line and 1.0 is all line and no hole. With derivatives, the units are somewhere in the vicinity of tens-of-pixels, a unit scale chosen to make available some reasonable default values that work pretty well in both environments. I'm open to change of course, but I'm going to wait for @pjcozzi to come back from GDC and have a chance to review this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said in person, these units have changed to be more pixel-like, and the default is now 1.0.

@emackey
Copy link
Contributor Author

emackey commented Apr 1, 2013

This is almost ready for merge. Didn't do anything with @mramato's comments on material uniform names yet, @pjcozzi or @bagnell any comments on that? I'll take a look at some of our other materials and see if we have any kind of standard names for things.

@emackey
Copy link
Contributor Author

emackey commented Apr 2, 2013

OK, I think this is ready for final review.

@mramato
Copy link
Contributor

mramato commented Apr 2, 2013

Specs?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 2, 2013

@emackey will look at this tomorrow.

@emackey
Copy link
Contributor Author

emackey commented Apr 2, 2013

Spec added for Grid material. The EllipsoidPrimitive gets no new parameters or tests now that the multi-res grid is reverted.

@@ -310,6 +310,11 @@
primitive.material = new Cesium.Material.fromType(scene.getContext(), 'Grass');
}

function applyGridMaterial(primitive, scene) {
Sandcastle.declare(applyGridMaterial); // For highlighting in Sandcastle.
primitive.material = new Cesium.Material.fromType(scene.getContext(), 'Grid');
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to actually set all the uniform values here, so that people can see what they can manipulate and play with it in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but in a previous note we discussed upgrading this demo for auto-discovery of all uniforms on all materials. This demo in its current form contains many materials with only their default uniforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, my reply to that would be to add uniform settings for all of the materials then, this demo overall is kind of useless without them (from a purely coding standpoint). That being said, if you don't have the bandwidth for it, we can just punt to a later date; no big deal.

@mramato
Copy link
Contributor

mramato commented Apr 2, 2013

  1. Might a user want to have the cell color be different than the grid color? Why not let them set cellColor instead of cellAlpha?
  2. It looks like if I set cellAlpha to 1.0 but set gridColor to have an alpha of 0.5, the result is not what I expect. The alpha of the gridColor seems to affect cellAlpha.

@mramato
Copy link
Contributor

mramato commented Apr 2, 2013

I realize my comments are all design/usability related. I'm just trying to determine what actual requirements for customers may be.

@emackey
Copy link
Contributor Author

emackey commented Apr 2, 2013

Perhaps gridColor is poorly named, it is the overall color and alpha for the entire GridMaterial, not just the lines themselves. I deliberately don't want to allow the cells to be a separate color. Reducing the alpha of the overall material makes the whole thing translucent (so the cells are affected by this as well as cellAlpha).

@mramato
Copy link
Contributor

mramato commented Apr 2, 2013

Then I would rename gridColor to just color, since that's what we use for every other material. cellAlpha is probably fine for a name, but we need to be clear in the doc that it's in addition to the overall color alpha.

@mramato
Copy link
Contributor

mramato commented Apr 2, 2013

All tests pass and Sandcastle demo works fine. This is fine with me assuming @pjcozzi is satisfied with the glsl, I'll let him merge when he's happy.

@emackey
Copy link
Contributor Author

emackey commented Apr 3, 2013

Updated Fabric wiki page.

@@ -46,7 +46,8 @@
e.center = ellipsoid.cartographicToCartesian(
Cesium.Cartographic.fromDegrees(-75.0, 40.0, 500000.0));
e.radii = new Cesium.Cartesian3(500000.0, 500000.0, 500000.0);
e.material = Cesium.Material.fromType(undefined, Cesium.Material.RimLightingType);
e.material = Cesium.Material.fromType(undefined, Cesium.Material.GridType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just add a new volume to show the grid material? I want to keep these examples to show the rim lighting and stripe material.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2013

@emackey did you test this on mobile?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2013

On AMD, there are some artifacts due to the partial derivatives (note the black where the ellipsoid overlaps the globe). Not sure how hard they will be to fix. I'm OK merging without them as this is a standard problem with partial derivatives in my experience.

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2013

@emackey just those few comments. This is good otherwise. Thanks! Materials are always welcome.

@emackey
Copy link
Contributor Author

emackey commented Apr 3, 2013

Just tested Grid material on Motorola Xoom. Works great, derivatives are enabled and all.

@emackey
Copy link
Contributor Author

emackey commented Apr 3, 2013

I'll live with the derivative artifacts for now. One option might be to detect the tangent points on the ellipsoid (or places on the material where the normal is 90 degrees from the camera) and color them separately. But that's for another day. This is ready for merge.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2013

Merging.

pjcozzi added a commit that referenced this pull request Apr 3, 2013
@pjcozzi pjcozzi merged commit 1c3c8aa into master Apr 3, 2013
@pjcozzi pjcozzi deleted the grid-material branch April 3, 2013 21:44
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.

4 participants