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

RaycasterHelper #30088

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

RaycasterHelper #30088

wants to merge 1 commit into from

Conversation

abernier
Copy link
Contributor

@abernier abernier commented Dec 10, 2024

Related issue: #23877

Description

a raycaster helper, from @gsimone https://github.com/gsimone/things?tab=readme-ov-file#raycasterhelper code

image image

@abernier abernier marked this pull request as draft December 10, 2024 12:51
Copy link

github-actions bot commented Dec 10, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.21
79.01
339.22
79.01
+16 B
+4 B
WebGPU 485.93
134.88
485.94
134.88
+16 B
+4 B
WebGPU Nodes 485.39
134.78
485.41
134.78
+16 B
+5 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.1
112.06
465.1
112.06
+0 B
+0 B
WebGPU 555.03
150.31
555.03
150.31
+0 B
+0 B
WebGPU Nodes 510.88
140.02
510.88
140.02
+0 B
+0 B

@abernier abernier marked this pull request as ready for review December 10, 2024 12:58
@abernier abernier mentioned this pull request Dec 10, 2024
@abernier abernier force-pushed the raycaster-helper branch 3 times, most recently from f55e1e1 to e5df3b9 Compare December 10, 2024 13:37
Co-authored-by: abernier <antoine.bernier@gmail.com>
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 10, 2024

Thinking about this now....what are the benefits of moving this into the main repository?

There is already a dedicated package where developers can import the helper. Besides, we want to be more strict in adding modules like RaycasterHelper so I'm not sure it's a necessary one to add.

In any event, RaycasterHelper should not be part of the core, see #23877 (comment).

@abernier
Copy link
Contributor Author

I have to say that I also asked myself the same question…

I would say (without being 100% sure) that the main benefit would probably being “officially” supported (and therefore maintained) by three.js — since Raycaster is an important class.

It also brings discoverability and overall consistency1

i let you decide obviously :)

In any event, RaycasterHelper should not be part of the core, see #23877 (comment).

I had hesitation but because Raycaster is a core class, that's the reason why I've made this choice, but no problen to move to jsm/helpers if we decide to integrate that helper 👍🏻

Footnotes

  1. because one could take the opposite approach and ask: “why not moving "ArrowHelper"/"BoxHelper"... to a third-party package?”

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 10, 2024

I also want to avoid the situation when developers encounter the same thing in different repositories. In my view, when RaycasterHelper is added to this repository, the original developer should do it and then remove the code from the previous location.

Besides, we often stated that we can't add "everything" into this repository. I understand it improves discoverability but it is simply not manageable for us. Having third-party repositories is totally fine and search engines makes sure developers find them. We from the core-team should make sure the most common and relevant modules are in the main repo. I don't think that includes RaycasterHelper.

@mrdoob
Copy link
Owner

mrdoob commented Dec 12, 2024

How about adding the example but importing from jsdelivr instead?

import { RaycasterHelper } from "https://cdn.jsdelivr.net/npm/@gsimone/three-raycaster-helper@0.1.0/dist/gsimone-three-raycaster-helper.esm.js";

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 12, 2024

I'm fine with that. Especially since we can tag the example as external so it should be clear it uses an external library.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 16, 2024

@abernier Do you mind updating the PR like suggested by @mrdoob ?

@abernier
Copy link
Contributor Author

sure, i will ;)

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.

4 participants