Skip to content

Conversation

georginahalpern
Copy link
Contributor

This PR introduces texturepreview to texture properties pane
image

Note that I am porting over logic from inspectorv1 and it can be simplified in future PR.
Note that the styling is not using tokens as this component hasn't yet undergone UX pass.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17236/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

Building or testing the playground has failed.

If the tests failed, results can be found here:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/testResults/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/?snapshot=refs/pull/17236/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17236/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

{texture.isCube ? (
<Toolbar className={classes.controls} aria-label="Cube Faces">
{["+X", "-X", "+Y", "-Y", "+Z", "-Z"].map((label, idx) => (
<ToolbarButton className={classes.controlButton} key={label} appearance={face === idx ? "primary" : "subtle"} onClick={() => setFace(idx)}>
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to manually track the "toggled" state and appearance rather than using ToolbarToggleButton?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just bc i didnt know about that :) -- im not seeing it in fluent docs, can u point me to it?

Comment on lines +75 to +76
previewCanvas.style.width = w + "px";
previewCanvas.style.height = h + "px";
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't seem like we would want to set the style width/height to the pixel width/height, it seems like they should be independent. I would think ideally you would just have the same kind of image "fit" options that you have with the Fluent Image component. I wonder if it would be helpful to actually use the Fluent Image and pass through a fit prop from the TexturePreivew component. I think we could do that if instead of rendering the directly, we called canvas.toDataURL() and then passed that in as the src on a Fluent Image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are some nuances to how the texture renders to the preview canvas context, i'll let @alexchuber chime in here!

Comment on lines +92 to +94
await WhenTextureReadyAsync(texture); // Ensure texture is loaded before grabbing size
const { w, h } = updatePreviewCanvasSize(previewCanvas); // Grab desired size
const data = await ApplyChannelsToTextureDataAsync(texture, w, h, face, channels); // get channel data to load onto canvas context
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to guard against the texture being disposed while this async work is happening? Should this component be watching the texture's onDisposeObservable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2025

georginahalpern and others added 2 commits October 3, 2025 20:41
…exturePreview.tsx

Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
…exturePreview.tsx

Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

Building or testing the sandbox has failed.

If the tests failed, results can be found here:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17236/merge/testResults/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

Building or testing the playground has failed.

If the tests failed, results can be found here:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/testResults/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@georginahalpern georginahalpern enabled auto-merge (squash) October 4, 2025 00:53
@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17236/merge/?snapshot=refs/pull/17236/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17236/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 4, 2025

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

Successfully merging this pull request may close these issues.

3 participants