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: 252 ocean component #363

Merged
merged 6 commits into from
Mar 13, 2024
Merged

feat: 252 ocean component #363

merged 6 commits into from
Mar 13, 2024

Conversation

JaimeTorrealba
Copy link
Member

closes #252

Copy link

stackblitz bot commented Mar 11, 2024

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

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.

This opens an "Ocean" of opportunities 😂. Sorry, only chance to drop a bad pun


extend({ Water })

const WaterRef = shallowRef()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const WaterRef = shallowRef()
const waterRef = shallowRef()

const _fog = scene.value.fog !== undefined

defineExpose({
WaterRef,
Copy link
Member

Choose a reason for hiding this comment

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

I would align this to the decision made on #160 (comment) to be consistent

await nextTick()
if (sunRef.value) {
const sunPosition = sunRef.value.material.uniforms.sunPosition.value
WaterRef.value.material.uniforms.sunDirection.value.copy(sunPosition)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
WaterRef.value.material.uniforms.sunDirection.value.copy(sunPosition)
waterRef.value.material.uniforms.sunDirection.value.copy(sunPosition)

const { onLoop } = useRenderLoop()

onLoop(({ delta }) => {
WaterRef.value.material.uniforms.time.value += delta
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
WaterRef.value.material.uniforms.time.value += delta
waterRef.value.material.uniforms.time.value += delta


<template>
<TresWater
ref="WaterRef"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ref="WaterRef"
ref="waterRef"

@@ -56,10 +57,16 @@ function getSunPosition(azimuth: number, elevation: number) {
const theta = MathUtils.degToRad(azimuth)
return new Vector3().setFromSphericalCoords(1, phi, theta)
}

defineExpose({
value: skyRef,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol I come with the idea and I forget it hahaha sorry I'll fix it :D

@alvarosabu
Copy link
Member

Screenshot 2024-03-11 at 18 28 30

Looks amazing! @JaimeTorrealba is sun direction always required?

@andretchen0
Copy link
Contributor

Currently reviewing. But just wanted to say that it looks great!

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.

Hey @JaimeTorrealba !

Just a few changes. Mostly typos and/or non-blocking.

Looks great!


| Prop | Description | Default |
| :------------------ | :------------------------------------------ | ------------------------------------------------------------------------------------------------ |
| **textureWidth** | With of the mirror texture | 512 |
Copy link
Contributor

Choose a reason for hiding this comment

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

With of the mirror texture

Width of the mirror texture

`<Ocean />` is a wrapper for the [Three.js `Water` add-on](https://threejs.org/examples/?q=ocean#webgl_shaders_ocean).

::: warning
Smoke componente comes with a default texture abstraction it needs to be wrapped by a Suspense component
Copy link
Contributor

Choose a reason for hiding this comment

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

Smoke componente

I think this should be one of

  • The <Ocean /> component
  • The Ocean component
  • <Ocean />
  • Ocean

I guess I prefer <Ocean />.

(Sometimes we use <Tag />. We should probably come up with a "rule" for ourselves.)

Also, maybe add a period at the end of the sentence.

All of the props of this component are not reactive
:::

| Prop | Description | Default |
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking)

Just in case this component slipped by you: you can use the CientosPropsTable like so:

<CientosPropsTable component-path="src/core/staging/Ocean.vue" :fields="['name', 'description', 'default']" />

That will create a formatted table like the one you've typed, but it'll stay updated with the JSDoc comments in the component's source code. (Unfortunately, it doesn't help with translations, though.)

No worries if you don't want to use it though. Just mark as "resolved".

Though if you have ideas for improving <CientosPropsTable />, I'm all ears.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't make it works, Andre. I don't know what happen (I have a new computer) the cientosPropsTable is just not working for me even after build the repo.

I got.

image

So I just use the manual approach, any idea what it could be?

Copy link
Contributor

Choose a reason for hiding this comment

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

any idea

As far as I can remember, problems I've run into in the past have been resolved by doing this:

  • Stop all running servers – including vitepress
  • Check out the git branch that has the new component
  • pnpm i
  • pnpm run build
  • pnpm run docs:dev

As long as the component is 'built', usually just stopping vitepress and restarting works – the plugin loads/parses the JSDoc metadata when the vitepress server starts.

So I just use the manual approach

No problem using the manual approach if that makes life easier.


### SKY

The `<Ocean />` work pair to pair with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
Copy link
Contributor

Choose a reason for hiding this comment

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

The <Ocean /> work pair to pair with the

The <Ocean /> component works hand in hand with the

### SKY

The `<Ocean />` work pair to pair with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
(`<Sky />` component is not required for making this component work)
Copy link
Contributor

Choose a reason for hiding this comment

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

(<Sky /> component is not required for making this component work)

(<Sky /> is not required for making this component work.)


### Fog

The `<Ocean />` component also react when there's [Fog](https://threejs.org/docs/index.html?q=fog#api/en/scenes/Fog) or [FogExp2](https://threejs.org/docs/index.html?q=fog#api/en/scenes/FogExp2) on your scene.
Copy link
Contributor

Choose a reason for hiding this comment

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

react

reacts

on your scene.

in your scene.


## Custom Geometry

You can usw a custom geometry by simple adding as a slot
Copy link
Contributor

Choose a reason for hiding this comment

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

You can usw a custom geometry by simple adding as a slot

Maybe:

You can use custom geometry by adding it as a child.

Rationale for "adding it as a child"

It seems to me that you, @JaimeTorrealba , as the creator of the component, added the <slot />.

That enables the user of the component to add a child.

toneMappingExposure: 1,
}

const newSunDirection = new Vector3(10, 0, 0)
Copy link
Contributor

@andretchen0 andretchen0 Mar 11, 2024

Choose a reason for hiding this comment

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

(Non-blocking)

It's a playground, but this could be removed since it isn't referenced anywhere.

@@ -0,0 +1,45 @@
<script setup lang="ts">
import { shallowRef, watch } from 'vue'
Copy link
Contributor

Choose a reason for hiding this comment

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

(Non-blocking)

This could be removed since the imports aren't used.

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.

I think my earlier comments about a few language changes weren't clear.

Below, I've put the entire sentences to copy/paste.

✌️

@@ -7,7 +7,7 @@
`<Ocean />` is a wrapper for the [Three.js `Water` add-on](https://threejs.org/examples/?q=ocean#webgl_shaders_ocean).

::: warning
Smoke componente comes with a default texture abstraction it needs to be wrapped by a Suspense component
`<Ocean />` componente comes with a default texture abstraction it needs to be wrapped by a Suspense component.
Copy link
Contributor

Choose a reason for hiding this comment

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

<Ocean /> comes with a default texture, so it needs to be wrapped in a Suspense component.

@@ -16,16 +16,16 @@ Smoke componente comes with a default texture abstraction it needs to be wrapped

### SKY

The `<Ocean />` work pair to pair with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
(`<Sky />` component is not required for making this component work)
The `<Ocean />` work hand to hand with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
Copy link
Contributor

Choose a reason for hiding this comment

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

<Ocean /> works hand in hand with the Sky component, detecting the position of the sun and reflecting on the water.

The `<Ocean />` work pair to pair with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
(`<Sky />` component is not required for making this component work)
The `<Ocean />` work hand to hand with the [Sky](https://cientos.tresjs.org/guide/staging/sky.html) component, detecting the position of the sun and reflecting on the water.
(<Sky /> is not required for making this component work.).
Copy link
Contributor

@andretchen0 andretchen0 Mar 12, 2024

Choose a reason for hiding this comment

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

(<Sky /> is not required for making this component work.)


  • Remove period at the end.
  • Make sure to keep the tick marks around <Sky /> when pasting. Otherwise, it disappears! ;)

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.

🌊

@JaimeTorrealba JaimeTorrealba merged commit 55ca2bf into main Mar 13, 2024
2 checks passed
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.

Water component
3 participants