Skip to content

Conversation

JustinTime42
Copy link
Contributor

This adds support for .glb files to be displayed by NFTMediaRenderer per issue #82.

@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2022

🦋 Changeset detected

Latest commit: 6693ce0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@thirdweb-dev/react Patch
@thirdweb-dev/sdk Patch
@thirdweb-dev/react-core Patch
thirdweb Patch
@thirdweb-dev/unity-js-bridge Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@JustinTime42
Copy link
Contributor Author

This is a fairly basic, initial implementation using Three.js. It allows the user to pass in some simple options like camera position, ambient light, and Frustrum. I'm open to feedback on other options we might want to support. I'm also looking into file formats other than .glb as well.

@jarrodwatts jarrodwatts requested a review from jnsdls December 22, 2022 23:59
@jnsdls
Copy link
Member

jnsdls commented Dec 27, 2022

@JustinTime42 thanks for this PR, at first glance this looks good though I had hoped we might potentially be able to rely on model viewer for the 3d rendering to avoid unnecessarily bundling three js etc (also supports more file types I believe)

Let me know if you want to take a stab at implementing this with model viewer instead, otherwise I can review it as is in the coming days and we can get this in as an immediate improvement. :)

@JustinTime42
Copy link
Contributor Author

@jnsdls Totally makes sense - thanks for the feedback!
The latest commit changes to using model-viewer for 3D models. For the moment, I've had to strip it down a bit and add several 'any' types to make it work. I'm running into some issues with typescript complaining about the model-viewer custom web element. I'm not very familiar with making custom web elements play nice with React and Typescript, but I can study up on that and get it figured out.

But I figured I'd commit what I have working so I can get feedback. If this looks like I'm generally on the right track I'll get to work fixing the types and getting everything working properly and safely.

@JustinTime42
Copy link
Contributor Author

@jnsdls
I've got the 3D rendering working pretty well now using model-viewer. It downloads the model-viewer script when the ModelViewer component loads so we avoid bundling it with our package or downloading it unless it's needed.

I have not been able to find an effective way of properly typing the element without installing model-viewer as a dependency, so I've left that as an 'any' type where I added model-viewer as a JSX intrinsic element. I'm definitely open to suggestions on this - I'm not a typescript expert yet so I might be missing something here.

Meanwhile this latest commit seems to work pretty well and I think it'll make a decent first version of supporting 3D models. Could you take a look and let me know what you think?

@joaquim-verges
Copy link
Member

@JustinTime42 looks like you ran npm install instead of yarn install btw - mind deleting the package-lock.json?

@JustinTime42
Copy link
Contributor Author

@joaquim-verges Oops - package-lock.json deleted.

@JustinTime42 JustinTime42 marked this pull request as draft January 8, 2023 04:43
@JustinTime42 JustinTime42 marked this pull request as ready for review January 10, 2023 05:09
Copy link
Member

@jnsdls jnsdls left a comment

Choose a reason for hiding this comment

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

thanks this is the right direction for sure!

@jnsdls
Copy link
Member

jnsdls commented Jan 27, 2023

made a couple of changes, @JustinTime42. Let me know if this all still works in the same ways that you've been testing it!

@JustinTime42
Copy link
Contributor Author

@jnsdls The only adjustment needed is that height:100% ends up being zero height and looks like so:
height100

I changed it to style={{ objectFit: 'contain', width: "100%" } which constrains the width to keep the Media column consistent and allows the height to maintain aspect ratio. It looks like this:
object-contain-width100

@nachoiacovino
Copy link
Contributor

Hey @JustinTime42 awesome job!

Now I can finally see the NFTs on my dashboard, one thing that happens is that I see them on the NFT List, but when I click on the the NFTs, I don't see them rendered on the NFT Drawer.

image

Basically it seems like it doesn't have a width, because adding either a width or min-width fixes it:
image

If we could figure out this last thing out we can ship this!

@jnsdls
Copy link
Member

jnsdls commented Jan 30, 2023

e2e test on CRA is failing

@nachoiacovino
Copy link
Contributor

/release-pr

@nachoiacovino
Copy link
Contributor

/release-pr

@nachoiacovino
Copy link
Contributor

shipping it @JustinTime42 🚀

@nachoiacovino nachoiacovino merged commit b59a31c into thirdweb-dev:main Feb 3, 2023
@github-actions github-actions bot mentioned this pull request Feb 3, 2023
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.

5 participants