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: edges #390

Merged
merged 13 commits into from
Dec 4, 2024
Merged

Conversation

damienmontastier
Copy link
Contributor

@damienmontastier damienmontastier commented Apr 29, 2024

This component provides an abstraction of EdgesGeometry from Three.js, <Edges> is specifically designed for rendering visible edges of objects in a scene graph. This enhances the visual quality by highlighting contours and providing a stylized appearance which contributes to the artistic aspect of 3D visualizations.

  • Easy to set up as it automatically derives geometry from its parent
  • You can simply wrap it around any Object3D, Mesh, or primitive to automatically apply edge rendering.
  • You can give to <Edges> a custom material

Local Playgroundpnpm run playground

Local Documentationpnpm run docs:dev

Copy link

stackblitz bot commented Apr 29, 2024

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

@JaimeTorrealba JaimeTorrealba self-requested a review April 30, 2024 12:21
Copy link
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.

Hi man, I'm glad you're back :), and thanks for this one

Did you know you can now open PRs directly to TresJs (you should be now part of the team)
Regarding the PR I didn't see any major changes, just little details, as always let me know

v-bind="$attrs"
>
<template v-if="hasChildren">
<slot />
Copy link
Member

Choose a reason for hiding this comment

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

Hi, here is better to use default slots (others component uses too)
for example


the TresLineBasicMaterial will act as fallback if any other slot has passed
Official docs here in case you want to check more about it: https://vuejs.org/guide/components/slots.html#fallback-content

So we can delete lines (23 and 25)

| Prop | Description | Default |
| :---------------- | :--------------------------------------------------- | ------------------------- |
| **color** | `THREE.Color` — Color of the edges. <br> More informations : [TresColor](https://docs.tresjs.org/api/instances-arguments-and-props.html#colors) — [THREE.Color](https://threejs.org/docs/#api/en/math/Color) | `#ff0000` |
| **threshold** | `number` — An edge is only rendered if the angle (in degrees) between the face normals of the adjoining faces exceeds this value | `1` |
Copy link
Member

Choose a reason for hiding this comment

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

Here I would add one more column with the LineSegments as a general (properly indicated in the description) or all the linesSegment props individually (better to be explicit, trust me we target several non-advance users)

Copy link
Member

Choose a reason for hiding this comment

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

Just for knowledge which props support LineSegments? I couldn't find any :o
The ThreeJs Docs sends me to Line (but this doesn't have any specific props, I was spectic line width or line-width but couldn't find any)

@alvarosabu alvarosabu added waiting for author feature New feature or request labels Sep 5, 2024
@andretchen0
Copy link
Contributor

Hey @damienmontastier ,

Are you still working on this?

@damienmontastier
Copy link
Contributor Author

Hey @andretchen0 !
I can continue on it, like the <Decal>

@andretchen0
Copy link
Contributor

Hey @andretchen0 ! I can continue on it, like the <Decal>

That'd be great!

Copy link

netlify bot commented Nov 28, 2024

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 79a507b
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/674ff94c0974170008c84188
😎 Deploy Preview https://deploy-preview-390--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.

@damienmontastier
Copy link
Contributor Author

damienmontastier commented Nov 28, 2024

hey @andretchen0 ! I just put a last modified version! ✌️

Previously @JaimeTorrealba had given me feedback regarding documentation here . I think there would be the possibility of improving <Edges> with Line2 or something! But currently the Line2 instance that is returned by a defineExpose returns a null value. Maybe in the future we could have a <Edges2> ?

Also, on the demo, I commented some code here, there's currently a bug with events pointer Tresjs/tres#801, so the code isn't functional at the moment.

@andretchen0
Copy link
Contributor

hey @andretchen0 ! I just put a last modified version! ✌️

Great!

But currently the Line2 instance that is returned by a defineExpose returns a null value. Maybe in the future we could have a <Edges2> ?

There's at least one open PR for <Line2>. I will check this out and get back to you.

Also, on the demo, I commented some code here, there's currently a bug with events pointer Tresjs/tres#801, so the code isn't functional at the moment.

We'll have to check that in the future, but it's not up to <Edges> to work around it, so don't worry about it here.

The events issue should be fixed with this PR. I'm not sure when it'll be merged though; it's part of a longer term project to bring portals to Tres core and I'm currently on the road and just working on smaller projects.

@andretchen0
Copy link
Contributor

But currently the Line2 instance that is returned by a defineExpose returns a null value.

The defineExpose issue will be fixed when this PR is merged: #523 . If you want to test on your branch, in src\core\shapes\Line2.vue, you just need to change :ref to ref here:

<template>
  <primitive
    :ref="lineRef"
    :object="line"
  />
</template>

@damienmontastier
Copy link
Contributor Author

Thanks @andretchen0 for your feedback!

I'm going to modify Line2 locally tomorrow! And I'll let you know!

Is this PR good for you? And so there will be an <Edges2> ?

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! Just a few comments.

Excited to have this one!

src/core/abstractions/Edges.vue Outdated Show resolved Hide resolved
docs/guide/abstractions/edges.md Outdated Show resolved Hide resolved
@andretchen0
Copy link
Contributor

Is this PR good for you?

It looks good! Docs demo is on-brand, too! I made a few review comments. Nothing major.

And so there will be an <Edges2> ?

If you want to implement one and there's an advantage over the PR here, I don't think anyone would say no. Though my personal preference at the moment would be to make one, good <Edge /> component for users.

Just FYI though, if you were comparing to Drei, their <Line /> and our <Line2 /> both use THREE.Line2. Drei use of THREE.Line2 here.

I called the Cientos' version <Line2 /> since it wraps THREE.Line2. And to avoid conflict in case we ever wanted to have a Cientos component based on THREE.Line.

Since THREE doesn't have Edges, I'm fine with calling your component here <Edges />, even if you want to use <Line2 /> or THREE.Line2 under the hood.

Maybe @alvarosabu wants to weigh in here.

@damienmontastier
Copy link
Contributor Author

If you want to implement one and there's an advantage over the PR here, I don't think anyone would say no. Though my personal preference at the moment would be to make one, good <Edge /> component for users.

Ok that's understandable!

I'm talking about a case where the user didn't want to use LineSegments (for which line-width doesn't exist, it's a fixed size). Instead, he wanted to use Line2, which embeds line-width modification. That's why I was talking about <Edges2 />.

@damienmontastier
Copy link
Contributor Author

Hey @andretchen0 !

I've just pushed the last modifications you asked for in the review.

@andretchen0
Copy link
Contributor

Hey @andretchen0 !

I've just pushed the last modifications you asked for in the review.

Looks good!

@andretchen0
Copy link
Contributor

If you want to implement one and there's an advantage over the PR here, I don't think anyone would say no. Though my personal preference at the moment would be to make one, good <Edge /> component for users.

Ok that's understandable!

I'm talking about a case where the user didn't want to use LineSegments (for which line-width doesn't exist, it's a fixed size). Instead, he wanted to use Line2, which embeds line-width modification. That's why I was talking about <Edges2 />.

Ah, gotcha.

Line width would be a useful addition, so if it wouldn't be too much work to use Line2 instead of LineSegments here, that'd be nice.

But I won't block the PR either way. It's good for me. Approved.

@damienmontastier
Copy link
Contributor Author

Hey @andretchen0 !

Yes, I think it will be simpler to create a <Edges2 /> for the use of Line2 and line-widths. This will avoid making <Edges> more complex.

Perfect! Can't wait for the next release!

@alvarosabu alvarosabu merged commit 60c200d into Tresjs:main Dec 4, 2024
5 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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants