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

Update ThreeJS #418

Merged
merged 16 commits into from
Sep 30, 2024
Merged

Update ThreeJS #418

merged 16 commits into from
Sep 30, 2024

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Sep 15, 2024

Currently a draft, but open for reviews
Related to #344

@arjxn-py
Copy link
Member Author

image
The changes doesn't look very pretty at the moment:

  • We've this plane visible which should be there but not be visible afaik
  • The colors of the model are a bit dimmed (the set color on the cube is sharp yellow, Maybe a lightning issue?)

Considering this a start, the next thing I plan are -

  • to get rid of some of @ts-ignores in the mainview
  • Update threeJS typings maybe
  • Identify above issues and resolve them

Copy link
Contributor

github-actions bot commented Sep 15, 2024

Preview PR at appsharing.space

@martinRenou
Copy link
Member

Thanks for starting this!

I believe the black mesh we are seeing here is the this._clippingPlaneMesh mesh object from the 3D view. IIRC it is not supposed to be visible, this one should only be used for the filling the gaps when clipping is enabled. I can have a deeper look at this later if you need.

@arjxn-py
Copy link
Member Author

I believe the black mesh we are seeing here is the this._clippingPlaneMesh mesh object from the 3D view. IIRC it is not supposed to be visible, this one should only be used for the filling the gaps when clipping is enabled. I can have a deeper look at this later if you need.

yes, i've seen this plane in the mainview. I shall try to look into it and also see if it can be fixed by making it not visible or something. I'll be happy to follow up on the same in case I would need you to have to take a look into the same :)

Copy link
Contributor

github-actions bot commented Sep 23, 2024

Integration tests repot: appsharing.space

@@ -218,7 +218,8 @@ export class MainView extends React.Component<IProps, IStates> {

this._renderer = new THREE.WebGLRenderer({
alpha: true,
antialias: true
antialias: true,
stencil: true
Copy link
Member

Choose a reason for hiding this comment

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

🚀 Neat

@arjxn-py
Copy link
Member Author

Bot please update snapshots.

@martinRenou
Copy link
Member

Bot please update snapshots!

@arjxn-py
Copy link
Member Author

Bot please update snapshots!

Update Galata References workflow is failing everytime xref: #438, I'll be happy to try on to fix it

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks a lot! That looks good to me and seems to work well

packages/base/package.json Show resolved Hide resolved
@@ -685,6 +686,7 @@ export class MainView extends React.Component<IProps, IStates> {
wireframe: this.state.wireframe
});
this._clippingPlaneMesh = new THREE.Mesh(planeGeom, planeMat);
this._clippingPlaneMesh.visible = this._clipSettings.enabled;
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this now that we set stencil: true on the renderer?

I'm fine letting this here though, it does sound sensible to not render the plane in anyway when the clipping is disabled.

Copy link
Member

Choose a reason for hiding this comment

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

^^ Just a question, not a blocker

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we still need this now that we set stencil: true on the renderer?

Yes, it's necessary to be there as it avoids the clip plane mesh from rendering itself fully and renders only the part of the plane which intersect with the geometry.

It avoids this from happening:

image

Copy link
Member

Choose a reason for hiding this comment

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

Ok then let's keep it! I'm unsure what changed in ThreeJS that requires this change, but the code looks more sensible now so that's good I guess

@martinRenou
Copy link
Member

Update Galata References workflow is failing everytime xref: #438, I'll be happy to try on to fix it

I commented there. Would you be able to try and fix the bot so we can get this PR green?

@arjxn-py
Copy link
Member Author

I commented there. Would you be able to try and fix the bot so we can get this PR green?

Thanks a lot 🚀
Yes, I'm on it.

@martinRenou
Copy link
Member

Bot please update snapshots!!

@martinRenou
Copy link
Member

Neat! I'll trigger the CI

@martinRenou martinRenou reopened this Sep 30, 2024
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinRenou martinRenou merged commit 0ba9a9e into jupytercad:main Sep 30, 2024
9 checks passed
@arjxn-py arjxn-py mentioned this pull request Oct 1, 2024
@arjxn-py arjxn-py deleted the update-threejs branch October 2, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants