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

perPositionHeight reversed material #7185

Closed
OmarShehata opened this issue Oct 24, 2018 · 10 comments · Fixed by #7188
Closed

perPositionHeight reversed material #7185

OmarShehata opened this issue Oct 24, 2018 · 10 comments · Fixed by #7188

Comments

@OmarShehata
Copy link
Contributor

In this Sandcastle, you can see the video is reversed. If you set perPositionHeight to false, it's correct.

I'm not sure why. Came up on the forum.

@mramato
Copy link
Contributor

mramato commented Oct 24, 2018

Perhaps the normals are wrong? @hpinkos any ideas?

@hpinkos
Copy link
Contributor

hpinkos commented Oct 24, 2018

Not sure what's going wrong. It shows up correctly using the primitive API and backwards using the entity API. I'll have to take a closer look to see where the difference is. It also shows up the correct way if you move the polygon to the western hemisphere.

This is the code I was testing with.

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var positions = Cesium.Cartesian3.fromDegreesArrayHeights([
            110,30,0, 
            110,40,0, 
            120,40,0, 
            120,30,0]);

var primitive = scene.primitives.add(new Cesium.Primitive({
    geometryInstances : [new Cesium.GeometryInstance({
        geometry : Cesium.PolygonGeometry.fromPositions({
            positions : positions,
            vertexFormat : Cesium.MaterialAppearance.MaterialSupport.TEXTURED.vertexFormat,
            perPositionHeight : true
        })
    })],
    appearance : new Cesium.MaterialAppearance({
        material : Cesium.Material.fromType('Image', {
            image: '../images/Cesium_Logo_Color.jpg'
        }),
        closed : false,
        translucent : false
    })
}));

var entity = viewer.entities.add({
    polygon: {
        hierarchy: positions,
        material: '../images/Cesium_Logo_Color.jpg',
        perPositionHeight: true,
        show: false
    }
});

var showPrimitive = true;
Sandcastle.addToolbarButton('toggle', function() {
    showPrimitive = !showPrimitive;
    primitive.show = showPrimitive;
    entity.polygon.show = !showPrimitive;
});

@mramato
Copy link
Contributor

mramato commented Oct 24, 2018

Even happens when the geometry is dynamic: hierarchy: Change the entity positions in your example to

new Cesium.CallbackProperty(function() {return positions;}, false),

So something specific to creating the geometry instance maybe?

@mramato
Copy link
Contributor

mramato commented Oct 24, 2018

Figured it out, the entity version is using CoplanarPolygonGeometry (Not sure if the bug is that it shouldn't be, or if there's a bug in CoplanarPolygonGeometry).

Here's an updated example showing the primitive issue as well:

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;

var positions = new Cesium.PolygonHierarchy(Cesium.Cartesian3.fromDegreesArrayHeights([
            110,30,0, 
            110,40,0, 
            120,40,0, 
            120,30,0]));

var primitive = scene.primitives.add(new Cesium.Primitive({
    geometryInstances : [new Cesium.GeometryInstance({
        geometry : new Cesium.CoplanarPolygonGeometry({
            polygonHierarchy : positions,
            vertexFormat : Cesium.MaterialAppearance.MaterialSupport.TEXTURED.vertexFormat,
            perPositionHeight : true
        })
    })],
    appearance : new Cesium.MaterialAppearance({
        material : Cesium.Material.fromType('Image', {
            image: '../images/Cesium_Logo_Color.jpg'
        }),
        closed : false,
        translucent : false
    })
}));

var entity = viewer.entities.add({
    polygon: {
        hierarchy: positions,
        material: '../images/Cesium_Logo_Color.jpg',
        perPositionHeight: true,
        show: true
    }
});

var showPrimitive = true;
Sandcastle.addToolbarButton('toggle', function() {
    showPrimitive = !showPrimitive;
    primitive.show = showPrimitive;
    entity.polygon.show = !showPrimitive;
    viewer.zoomTo(entity);
});

@hpinkos
Copy link
Contributor

hpinkos commented Oct 24, 2018

Oh right. I'm not sure I would necessarily call this a bug because the orientation of CoplanarPolygonGeometry is more or less treated as arbitrary, but we could do a better job guessing which side should be "forwards" based on whether the face is pointing roughly towards or away from the globe.

@OmarShehata
Copy link
Contributor Author

Shouldn't the winding order of the vertices determine which direction the polygon is facing? That way the user in this case has control over it as opposed to having to deal with that Cesium guesses.

@GatorScott
Copy link

Should treat like any other polygon and use winding order to define "front". Maybe I want to display something in the sky viewed from near the surface of the planet.

@hpinkos
Copy link
Contributor

hpinkos commented Oct 24, 2018

Winding order is likely why the polygon in the eastern hemisphere was backwards but if you negated the longitude values to put it in the western hemisphere it was forwards. Cesium uses counter-clockwise rotation for polygon positions and the input positions from that sample are clockwise.

However, our PolygonGeometry will detect if the positions are CW and reverse them automatically so both with as expected in Cesium. Whether CCW or CW is treated as the "front" varies depending on the platform. To make CoplanarPolygonGeometry have roughly the same behavior as PolygonGeometry (which is needed since they're both used by the Entity polygon) I added a check to see if the normal is pointing into or away from the globe. #7188

I suppose I could put this behind a flag if someone using the CoplanarPolygonGeometry directly doesn't want this behavior.

@hpinkos
Copy link
Contributor

hpinkos commented Oct 24, 2018

Upon taking a closer look at the code, CoplanarPolygonGeometry needs to remain winding order indifferent at this time. Winding order gets complicated because of our support for polygons with holes. It is certainly something we can revisit in the future, but it's not a common use case and not a priority at this time.

@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/d/msg/cesium-dev/CSQeUtcSj_4/_ridnkKxCAAJ

If this issue affects any of these threads, please post a comment like the following:

The issue at #7185 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

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

Successfully merging a pull request may close this issue.

5 participants