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

splitLongitude produces NaN texture coordinates #7406

Closed
OmarShehata opened this issue Dec 11, 2018 · 19 comments · Fixed by #7452
Closed

splitLongitude produces NaN texture coordinates #7406

OmarShehata opened this issue Dec 11, 2018 · 19 comments · Fixed by #7452

Comments

@OmarShehata
Copy link
Contributor

OmarShehata commented Dec 11, 2018

worldRectangle = scene.primitives.add(new Cesium.Primitive({
        geometryInstances : new Cesium.GeometryInstance({
            geometry : new Cesium.RectangleGeometry({
                rectangle : Cesium.Rectangle.fromDegrees(-180.0, -90.0, 180.0, 90.0),
                vertexFormat : Cesium.EllipsoidSurfaceAppearance.VERTEX_FORMAT
            })
        }),
        appearance : new Cesium.EllipsoidSurfaceAppearance({
            aboveGround : false
        }),
        show : false
    }));

I've discovered that the texture coordinates attribute has NaN's in it. If I put the following check in Source/Core/AttributeCompression.js everything works fine:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/AttributeCompression.js#L308-L316

if (isNaN(textureCoordinates.x) || isNaN(textureCoordinates.y)) {
       return 0;
}
@mramato
Copy link
Contributor

mramato commented Dec 11, 2018

May not be the same problem. I can run the tests, but the Materials demo crashes.

@likangning93
Copy link
Contributor

Looks like under linux it just kind of freezes after a few frames.

@alx696
Copy link

alx696 commented Dec 12, 2018

Ubuntu 18.04 Chrome71 crash after the Globe show.

@mramato
Copy link
Contributor

mramato commented Dec 12, 2018

@lilleyse @bagnell this is a huge issue that we need to address ASAP. Who would be the best person to look at this?

@lilleyse
Copy link
Contributor

@OmarShehata is going to look into this a bit today. It should be easy to identify which geometry or material is causing the problem in this demo.

@OmarShehata
Copy link
Contributor Author

OmarShehata commented Dec 12, 2018

Preliminary results: I'm not sure why yet, but it seems to be having trouble with worldRectangle.

worldRectangle = scene.primitives.add(new Cesium.Primitive({
        geometryInstances : new Cesium.GeometryInstance({
            geometry : new Cesium.RectangleGeometry({
                rectangle : Cesium.Rectangle.fromDegrees(-180.0, -90.0, 180.0, 90.0),
                vertexFormat : Cesium.EllipsoidSurfaceAppearance.VERTEX_FORMAT
            })
        }),
        appearance : new Cesium.EllipsoidSurfaceAppearance({
            aboveGround : false
        }),
        show : false
    }));

If I change the line:

rectangle : Cesium.Rectangle.fromDegrees(-180.0, -90.0, 180.0, 90.0),

To

rectangle : Cesium.Rectangle.fromDegrees(-180.0, -80.0, 180.0, 80.0),

It works fine. Sandcastle with above modification.

@OmarShehata
Copy link
Contributor Author

The other way to fix it is to comment out the appearance, so the following works:

worldRectangle = scene.primitives.add(new Cesium.Primitive({
        geometryInstances : new Cesium.GeometryInstance({
            geometry : new Cesium.RectangleGeometry({
                rectangle : Cesium.Rectangle.fromDegrees(-180.0, -90.0, 180.0, 90.0),
                vertexFormat : Cesium.EllipsoidSurfaceAppearance.VERTEX_FORMAT
            })
        }),
        //appearance : new Cesium.EllipsoidSurfaceAppearance({
        //    aboveGround : false
        //}),
        show : false
    }));

I've discovered that the texture coordinates attribute has NaN's in it. If I put the following check in Source/Core/AttributeCompression.js everything works fine:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/AttributeCompression.js#L308-L316

if (isNaN(textureCoordinates.x) || isNaN(textureCoordinates.y)) {
       return 0;
}

For some reason, this st attribute list contains NaN values:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/GeometryPipeline.js#L1403-L1412

@OmarShehata
Copy link
Contributor Author

The issue seems specifically to be in the compressVertices step. Another way to make this Sandcastle work is simply to pass compressVertices : false to the worldPrimitive.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 12, 2018

Cool, texture coordinate NaNs is a very fixable problem. I guess this is something previous versions of Chrome tolerated but this latest release does not.

@OmarShehata
Copy link
Contributor Author

Yup! That's the good news. I was worried it was some very specific obscure Chrome 71 WebGL issue, but you can see the NaNs are present in Firefox as well, and it just doesn't crash there.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 12, 2018

I can probably take a look at this next week if no one else gets to it first. The texture coordinate calculation happens here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/RectangleGeometryLibrary.js#L56-L68

@OmarShehata
Copy link
Contributor Author

OmarShehata commented Dec 12, 2018

I was wrong about compressVertices. It gets NaN's before that, it's just that it only crashes during that step.

@hpinkos I just checked RectangleGeometryLibrary and the produces coordinates there are clean.

The NaNs are introduced in GeometryPipeline.splitLongitude(instances[i]); Commenting out this line (or just running the Sandcastle with scene3DOnly proves this is the culprit.

That also explains why it was fixed when we made it smaller (I think).

@OmarShehata
Copy link
Contributor Author

@hpinkos if you're more familiar with that class, I can stop there and let you take it from here.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 12, 2018

We have so many problems caused by that splitLongitude function. @OmarShehata keep poking around at it and let me know if you get stuck. My guess would be something's going weird in this function: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/GeometryPipeline.js#L1919

@mramato
Copy link
Contributor

mramato commented Dec 13, 2018

They released a new minor version update for Chrome last night and it no longer crashes on my system. Maybe it was a regression on their end? Can others confirm?

I still maintain that Chrome 71 was garbage, this and #7388 seem to confirm that.

@OmarShehata
Copy link
Contributor Author

@mramato it's still true that splitLongitude is spitting out NaN's that are being sent to the GPU in the texture coordinate attribute buffer. If it doesn't crash anymore it might not be as big of a priority but probably still something we should fix.

I'm guessing the crash was not intentionally on Chrome's side because the debug log says something like "Application tried to access memory and was denied" as opposed to "NaN values are not allowed here".

@hpinkos hpinkos changed the title Materials Sandcastle crashes in Chrome splitLongitude produces NaNs Dec 13, 2018
@hpinkos hpinkos changed the title splitLongitude produces NaNs splitLongitude totally wrong for texture coordiantes Dec 13, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Dec 13, 2018

Re-targeted this issue to fix the texture coordinate computation in splitLongitude. You can see from the gif I posted in the first comment that it's doing something completely crazy. The numbers its generating aren't within the [0, 1] range.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 13, 2018

Only seems to be a problem with GroundPrimitive. They didn't show up at all prior to 1.49. We fixed it for 1.49 with #6951 so they display but the texture coordinates are wrong.

@hpinkos
Copy link
Contributor

hpinkos commented Dec 13, 2018

Just kidding, the GroundPrimitive texture thing is unrelated to splitLongitude. I'm going to open a separate issue. We'll keep this one for fixing the NaN thing.

@hpinkos hpinkos changed the title splitLongitude totally wrong for texture coordiantes splitLongitude produces NaN texture coordinates Dec 13, 2018
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.

6 participants