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

feat: 170 use gltf exporter #290

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Conversation

JaimeTorrealba
Copy link
Member

closes #170

Copy link

stackblitz bot commented Nov 25, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 571c554
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/65a3f5f8b904890008bd8dd1
😎 Deploy Preview https://deploy-preview-290--cientos-tresjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
@alvarosabu alvarosabu added the v4 label Nov 28, 2023
Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Sorry chamo, can't pass this with an any, isn't the type TresObject3D helpful here?

src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Nice job! Works as expected. For me, there are just a few points to look over.

src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
src/core/misc/useGLTFExporter.ts Outdated Show resolved Hide resolved
docs/.vitepress/config.ts Outdated Show resolved Hide resolved
docs/guide/misc/use-gltf-exporter.md Outdated Show resolved Hide resolved
docs/guide/misc/use-gltf-exporter.md Outdated Show resolved Hide resolved
@alvarosabu
Copy link
Member

Hey @andretchen0 could you check if all the conversations where solved please?

@andretchen0
Copy link
Contributor

andretchen0 commented Dec 15, 2023

Hey @andretchen0 could you check if all the conversations where solved please?

@alvarosabu : Will do.

@andretchen0
Copy link
Contributor

andretchen0 commented Dec 15, 2023

Hey @JaimeTorrealba

It looks like these change requests are still pending:

😉

@JaimeTorrealba
Copy link
Member Author

@andretchen0 sorry I haven't paid attention to this notification. Sorry.

Done :). And thanks, as you may guest I'm not good with the English grammar

@andretchen0
Copy link
Contributor

@andretchen0 sorry I haven't paid attention to this notification. Sorry.

Done :). And thanks, as you may guest I'm not good with the English grammar

No worries man. You're doing a lot better than me in my second language, lol.

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@alvarosabu
Copy link
Member

Awesome, @JaimeTorrealba merge latest main to resolve conflicts and we are ready to merge this PR

@JaimeTorrealba
Copy link
Member Author

@alvarosabu done :D

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Since merging in main, the playground example is broken. useTweakPane will need to be converted to Leches.

@@ -0,0 +1,63 @@
<script setup lang="ts">
import { TresCanvas, useRenderLoop } from '@tresjs/core'
import { CameraControls, useTweakPane, useGLTFExporter } from '@tresjs/cientos'
Copy link
Contributor

Choose a reason for hiding this comment

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

This errors out for me.

I believe it's because latest Cientos no longer exports useTweakPane.

const donutGeometry = new TorusGeometry(1, 0.5, 16, 32)
const boxRef = shallowRef()

const { pane } = useTweakPane()
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there's no useTweakPane this should probably be converted to Leches.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're complete right, thanks for pointing Andre. It's already fix

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Nice job. Works as advertised! Just one small typo to fix.

I'll approve the PR either way, but as an optional addition, could we also show how to download the whole Tres scene? No problem if you'd rather keep it simple.

docs/guide/misc/use-gltf-exporter.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Great! Thanks for adding the scene example.

@JaimeTorrealba
Copy link
Member Author

@alvarosabu please your approval here is necessary :)

@JaimeTorrealba
Copy link
Member Author

@alvarosabu sorry for tag you again, I would like to merge this one :)

@JaimeTorrealba JaimeTorrealba merged commit 411d631 into main Jan 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exporting scene
3 participants