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

Fix ability to "unset" material image #9644

Merged
merged 11 commits into from
Jul 8, 2021
Merged

Fix ability to "unset" material image #9644

merged 11 commits into from
Jul 8, 2021

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jun 28, 2021

Will fix #9385. Not complete, but I had some questions about this block of code.

  1. I added something to the material update function (which comes from createTexture2DUpdateFunction) that can handle when the value of an image is undefined. However, I'm unclear about the intended behavior of this function when it has no texture. It seems that if a texture is not defined, it wants to define some default texture anyway -- is that what we want?
  2. Regarding "czm_defaultImage": is this default image being created anywhere? The function just returns early if uniformValue === Material.DefaultImageId, so I'm not sure if the texture property is actually being set.

I'm still addressing a bug where if you alternate between czm_defaultImage and undefined, some random image appears -- not sure what it is.

cc: @lilleyse @ebogo1

@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@j9liu
Copy link
Contributor Author

j9liu commented Jun 30, 2021

I'm still addressing a bug where if you alternate between czm_defaultImage and undefined, some random image appears -- not sure what it is.

The most recent change should have taken care of this. However, the image appears when toggling between undefined and either the "does_not_exist" or empty quote options.

I'm currently writing tests for the changes to Material, but I ran into some concerns. When I declare this Material in a test:

var material = Material.fromType(Material.ImageType, {
        image: "./Data/Images/Green.png",
        color: Color.WHITE,
      });

it will render the color white even though "./Data/Images/Green.png" should set the value to [0, 255, 0, 255].
I went back to the example Sandcastle in the original issue, and changed the image property of the material so that it began with the Cesium logo displayed. But for the first few frames of rendering the rectangle was blank, and after three frames it finally showed the image. The logic in the Material.prototype.update calls its updateFunctions after dealing with loadedImages, so this is probably the issue -- I'm looking into it.

  • Even if I rearrange the order of this, though, what if Resource.fetchImage just takes a long time? Maybe I need to write my unit tests in a way that account for this?

Also, not related to this issue / PR, but the "does not destroy default material" test in MaterialSpec has no expectations -- this seems like a mistake?

@j9liu
Copy link
Contributor Author

j9liu commented Jul 1, 2021

@lilleyse - I think this PR is ready for review. I put the changes under 1.84 in CHANGES.md because I figure we'll merge this after the release today.

Sorry about the 20+ commits; they happened when I was git pull-ing from master. If there's any way I can avoid that in the future let me know!

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Good start @j9liu! I confirmed that this fixes all the cases in the issue. The unit tests are solid. I just have some suggestions for how the code could be organized.

Source/Scene/Material.js Outdated Show resolved Hide resolved
Source/Scene/Material.js Outdated Show resolved Hide resolved
Source/Scene/Material.js Show resolved Hide resolved
Specs/Scene/MaterialSpec.js Outdated Show resolved Hide resolved
Specs/Scene/MaterialSpec.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

lilleyse commented Jul 3, 2021

Sorry about the 20+ commits; they happened when I was git pull-ing from master. If there's any way I can avoid that in the future let me know!

To fix the commit history:

  • Create a branch off master
  • Cherry-pick the commits you want to keep
  • Force push to unset-material-image (instructions for that)

In general if you're trying to merge master into your branch, use git merge instead of git pull

@j9liu
Copy link
Contributor Author

j9liu commented Jul 6, 2021

@lilleyse - I followed your comments, let me know if anything needs changing!

Source/Scene/Material.js Outdated Show resolved Hide resolved
Source/Scene/Material.js Outdated Show resolved Hide resolved
Source/Scene/Material.js Show resolved Hide resolved
Source/Scene/Material.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Jul 8, 2021

@lilleyse - how does this look now?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Really close now. The code comments are very helpful. Just one last comment about destroying the texture in one case.

Source/Scene/Material.js Outdated Show resolved Hide resolved
Source/Scene/Material.js Show resolved Hide resolved
@lilleyse
Copy link
Contributor

lilleyse commented Jul 8, 2021

Thanks @j9liu!

@lilleyse lilleyse merged commit f6dc9d1 into master Jul 8, 2021
@lilleyse lilleyse deleted the unset-material-image branch July 8, 2021 21:52
This pull request was closed.
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.

Unable to unset image uniform for Material
3 participants