Skip to content
This repository was archived by the owner on Feb 1, 2026. It is now read-only.

feat(Sky): add Sky#213

Merged
JaimeTorrealba merged 13 commits intomainfrom
feature/106-sky
Sep 17, 2023
Merged

feat(Sky): add Sky#213
JaimeTorrealba merged 13 commits intomainfrom
feature/106-sky

Conversation

@andretchen0
Copy link
Copy Markdown
Contributor

@andretchen0 andretchen0 commented Sep 15, 2023

#106

Playground demo at: /staging/sky

If this looks ok, I'll add the docs.


Question: Template uniforms lint --fix problem

Is there a workaround for this?

pnpm run lint --fix throws an error and "fixes" the kebab case on the last line:

  <primitive
    :object="skyImpl"
    :material-uniforms-mieCoefficient-value="props.mieCoefficient"

transforming it into this

  <primitive
    :object="skyImpl"
    :material-uniforms-mie-coefficient-value="props.mieCoefficient"

but the "fix" causes an error to be thrown in the browser, when run.

I solved the problem by using watch, but if there's a way around it, I'd love to know.

Thanks!

@andretchen0 andretchen0 linked an issue Sep 15, 2023 that may be closed by this pull request
Closed
4 tasks
@netlify
Copy link
Copy Markdown

netlify Bot commented Sep 15, 2023

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit dbc191d
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/65061e230ed1ee0008ef368a
😎 Deploy Preview https://deploy-preview-213--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.

@alvarosabu
Copy link
Copy Markdown
Member

Hi @andretchen0 thanks for this, regarding the lint, yes, a similar thing happens with shadow.mapSize.width here in the lint config https://github.com/Tresjs/configs/blob/v0.2.0/packages/eslint-config-vue/index.js#L137-L139

Please add a PR on the repo with the exception(s) you need

@andretchen0
Copy link
Copy Markdown
Contributor Author

Thanks, @alvarosabu . Will do.

Please add a PR on the repo with the exception(s) you need

Do you prefer focused, case-by-case exceptions?

'material-uniforms-mieCoefficient-value', 'material-uniforms-mieDirectionalG-value', 'material-uniforms-sunPosition-value'

But setting uniforms is common in Three, so maybe a more general approach is useful? Maybe ...

-uniforms-

Looking at the linter source, it appears that the general approach would work, but it's not explicitly documented at the moment.

@alvarosabu
Copy link
Copy Markdown
Member

Mmm, I see, with uniforms is not scalable 🤔

@andretchen0
Copy link
Copy Markdown
Contributor Author

@JaimeTorrealba

Following the comments above, I've moved the watched props to the template – that's preferable to me as well.

With the current linter setup, the component will break if someone runs pnpm run lint --fix, so we'll need to sort out the linter (or patch Tres) before merging into main.

But let me know about the component. If the API is ok (I used the Three Sky API, minus exposure, which modifies the renderer), I'll add the docs.

@andretchen0
Copy link
Copy Markdown
Contributor Author

andretchen0 commented Sep 15, 2023

Opened a PR for the linter hyphenation fix.

If that's merged, Sky should be ready to go.

Copy link
Copy Markdown
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

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

Excellent work, I just have little details, when you fix them I'll approve them but no merge this until the Tresjs/configs#3 it's finished

Comment thread docs/guide/staging/sky.md Outdated
Comment thread playground/src/pages/staging/SkyDemo.vue
Comment thread playground/src/pages/staging/SkyDemo.vue Outdated
Comment thread playground/src/pages/staging/SkyDemo.vue Outdated
@andretchen0
Copy link
Copy Markdown
Contributor Author

@JaimeTorrealba

I just have little details

All good stuff. Thanks for the feedback.

when you fix them I'll approve them but no merge this until the Tresjs/configs#3 it's finished

The config changes were merged – though it seems like the CI linter here on Github hasn't been updated.

I think Sky is ready to merge. Otherwise, let me know what changes need to be made and I'll update the component.

@andretchen0 andretchen0 marked this pull request as ready for review September 16, 2023 21:33
Copy link
Copy Markdown
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

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

Thanks for this
We really appreciate it

@JaimeTorrealba
Copy link
Copy Markdown
Member

@alvarosabu This is ready, but I don't want to merge it until the linter pipeline get update.
Please let me know about this

@alvarosabu
Copy link
Copy Markdown
Member

@JaimeTorrealba I released a new version of the linter, you guys should be able to add it

@JaimeTorrealba JaimeTorrealba merged commit 1fb1673 into main Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sky

3 participants