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(app): Add holographic material to cientos #370

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

JaimeTorrealba
Copy link
Member

No description provided.

Copy link

stackblitz bot commented Mar 25, 2024

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

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 !

Looks great! Nice addition!

Just a few changes.

Copy link
Contributor

@andretchen0 andretchen0 Mar 25, 2024

Choose a reason for hiding this comment

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

(non-blocking)

Cool effect! I like it!

Here's how the demo shows up for me:

Screenshot 2024-03-26 at 00 15 41

Can we show it off more by zooming in on the face a little and centering it vertically?

And add a <Levioso />?

Maybe like this?

Screenshot 2024-03-26 at 00 14 58
<script setup lang="ts">
import { shallowRef, watch } from 'vue'
import { TresCanvas } from '@tresjs/core'
import { HolographicMaterial, OrbitControls, useGLTF, Levioso } from '@tresjs/cientos'

const path = 'https://raw.githubusercontent.com/'
+ 'Tresjs/assets/main/models/gltf/aku-aku/AkuAku.gltf'
const { scene } = await useGLTF(path)

const holographicMaterialRef = shallowRef()

watch(holographicMaterialRef, (value) => {
  scene.traverse((child) => {
    if (child.isMesh) {
      child.material.dispose()
      child.material = value.root
    }
  })
})
</script>

<template>
  <TresCanvas clear-color="#333">
    <TresPerspectiveCamera :position="[0, 0, 6]" />
    <Levioso :speed="5">
      <primitive
        :object="scene"
        :position-y="-2.5"
      >
        <HolographicMaterial ref="holographicMaterialRef" />
      </primitive>
    </Levioso>
    <OrbitControls />
  </TresCanvas>
</template>

<HolographicMaterialDemo />
</DocsDemo>

A simple to use holographic material for threejs
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)

Maybe format as a header and change "threejs" to "TresJS"?

## A simple to use holographic material for TresJS


:::info
This is an integration of the Anderson Mancini [threejs-vanilla-holographic-material](https://github.com/ektogamat/threejs-vanilla-holographic-material). All credits for him.
:::
Copy link
Contributor

@andretchen0 andretchen0 Mar 25, 2024

Choose a reason for hiding this comment

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

Change to:

This component ports Anderson Mancini's [threejs-vanilla-holographic-material](https://github.com/ektogamat/threejs-vanilla-holographic-material) to TresJS. All credit goes to him.


### You can also replace the material of an existing mesh like this:

<<< @/.vitepress/theme/components/GlassMaterialDemo.vue{3,9-12}
Copy link
Contributor

Choose a reason for hiding this comment

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

This adds the wrong demo, I think. It should be

<<< @/.vitepress/theme/components/HolographicMaterialDemo.vue

<primitive :object="scene" />
<Sphere :visible="false">
<HolographicMaterial ref="holographicMaterialRef" />
</Sphere>
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)

Are you using the <Sphere> to have something to attach the material to, so you can get the ref? Currently anyway, you can attach the material to the <primitive> and get the ref there.

      <primitive :object="scene">
        <HolographicMaterial ref="holographicMaterialRef" />
      </primitive>


| Prop | Description | Type | default |
| :--------------------- | :------------------------------------------------------------------ | --------------------------------------------------- | --------- |
| **fresnelAmount** | Controls the value of the Fresnel effect. Ranges from 0.0 to 1.0. | `Number` | 0.45 |
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)

I'd drop "Controls the" and "Specifies the" in the descriptions. The descriptions read fine without the extra words. And shorter descriptions makes them easier to scan.

E.g.,

  • "Value of the Fresnel effect. Ranges from 0.0 to 1.0."
  • "Opacity of the Fresnel effect. Ranges from 0.0 to 1.0."
  • "Size of the scanlines. Ranges from 1 to 15."


<<< @/.vitepress/theme/components/GlassMaterialDemo.vue{3,9-12}

## props
Copy link
Contributor

Choose a reason for hiding this comment

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

props => Props

We've been capitalizing it in other pages.

| **hologramColor** | Specifies the color of the hologram. | `String` | "#00d5ff" |
| **enableBlinking** | Enables or disables the blinking effect. | `Boolean` | true |
| **hologramOpacity** | Specifies the opacity of the hologram. | `Number` | 1.0 |
| **enableBlinking** | Enables or disables the blinking effect. | `Boolean` | true |
Copy link
Contributor

@andretchen0 andretchen0 Mar 25, 2024

Choose a reason for hiding this comment

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

This line (starting with enableBlinking) can be deleted. It was added to the table twice.

</template>
```

### You can also replace the material of an existing mesh like this:
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)

To me, it looks a little funny to have this long header floating between 2 code blocks. Maybe change to:

### Replace a mesh's material

You can also replace the material of an existing mesh like this:

} from 'three'
import type { Side, Blending } from 'three'

// Original author Anderson Mancini - https://github.com/ektogamat/threejs-vanilla-holographic-material
Copy link
Contributor

Choose a reason for hiding this comment

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

@JaimeTorrealba @alvarosabu

Is this sufficient for the license?

I have zero legal knowledge, but reading over the MIT license from the repo, it sounds like the whole text needs to accompany Anderson Mancini's code.

Here's the salient bit:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

@JaimeTorrealba JaimeTorrealba self-assigned this Mar 26, 2024
@alvarosabu alvarosabu added p3-significant High-priority enhancement (priority) feature New feature or request labels Apr 2, 2024
@JaimeTorrealba
Copy link
Member Author

I already implemented all the feedbacks, as always thanks for take your time to do it, and the quality of them, it makes me grow as a professional and have a better library.

Btw the levioso advice it was just perfect 👍

@andretchen0
Copy link
Contributor

I already implemented all the feedbacks, as always thanks for take your time to do it, and the quality of them, it makes me grow as a professional and have a better library.

That's really nice to hear. Thanks!

@JaimeTorrealba JaimeTorrealba merged commit f897c33 into main Apr 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request p3-significant High-priority enhancement (priority)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants