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

Add HTMLImageElement for Material uniform #6729

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jun 25, 2018

Replaces #5857


From #5857: In order to make an animation by many images,we must know every image status in the process,so I change the timeline to the next time after the current image loaded.

material.uniforms.image = loadedImage;

@cesium-concierge
Copy link

Signed CLA is on file.

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


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

🌍 🌎 🌏

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 25, 2018

@ggetz can you review?

@mramato
Copy link
Contributor

mramato commented Jun 25, 2018

CHANGES?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 25, 2018

@mramato updated

@mramato
Copy link
Contributor

mramato commented Jun 25, 2018

Also, can we add a unit test for this? And sorry if I'm being a little daft, but what is the use case that didn't work before? Can we at least have a Sandcastle link to test?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 25, 2018

@mramato it fixes this:

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

var image2 = new Image(100, 200);
image2.src = '../images/Cesium_Logo_Color.jpg';

var material = Cesium.Material.fromType('Image', {
    image: '../images/Cesium_Logo_overlay.png'
});

viewer.scene.primitives.add(new Cesium.Primitive({
    geometryInstances : new Cesium.GeometryInstance({
        geometry : new Cesium.RectangleGeometry({
            rectangle : Cesium.Rectangle.fromDegrees(-120.0, 30.0, -110.0, 40.0),
            vertexFormat : Cesium.EllipsoidSurfaceAppearance.VERTEX_FORMAT
        })
    }),
    appearance : new Cesium.EllipsoidSurfaceAppearance({
        material : material
    })
}));

Sandcastle.addToolbarButton('Change image', function() {
    material.uniforms.image = image2;
});

In master, the image doesn't change when you press the button. In this branch it does.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 25, 2018

I'm honestly not sure what the best way to write a unit test for this would be

@mramato
Copy link
Contributor

mramato commented Jun 26, 2018

I think there's still a bug where updating the src attribute on an existing image doesn't reflect the changes, probably because we are only checkin Image === Image2 and not Image.src === Image2.src as well:

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

var image = new Image(100, 200);
image.src = '../images/Cesium_Logo_Color.jpg';

var material = Cesium.Material.fromType('Image', {
    image: image
});

viewer.scene.primitives.add(new Cesium.Primitive({
    geometryInstances : new Cesium.GeometryInstance({
        geometry : new Cesium.RectangleGeometry({
            rectangle : Cesium.Rectangle.fromDegrees(-120.0, 30.0, -110.0, 40.0),
            vertexFormat : Cesium.EllipsoidSurfaceAppearance.VERTEX_FORMAT
        })
    }),
    appearance : new Cesium.EllipsoidSurfaceAppearance({
        material : material
    })
}));

Sandcastle.addToolbarButton('Change image', function() {
    image.onload = function(){
        material.uniforms.image = image;
    };
    image.src = '../images/Cesium_Logo_overlay.png';
});

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 26, 2018

@mramato I would argue that's a different bug. I'm not sure how we would even detect that. We also don't detect when the content of a canvas changes: #5080

@hpinkos
Copy link
Contributor Author

hpinkos commented Jun 26, 2018

@mramato I added your example to #5080 because that seems really closely related to the canvas changing. I think this PR should be merged as is.

@mramato
Copy link
Contributor

mramato commented Jun 26, 2018

Fair enough.

@mramato mramato merged commit ea95fe8 into master Jun 26, 2018
@mramato mramato deleted the htmlImageElementMaterial branch June 26, 2018 15:51
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.

3 participants