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

ArcballControls is using .clone() unnecessarily #22823

Closed
WestLangley opened this issue Nov 12, 2021 · 6 comments · Fixed by #22856 or #23777
Closed

ArcballControls is using .clone() unnecessarily #22823

WestLangley opened this issue Nov 12, 2021 · 6 comments · Fixed by #22856 or #23777
Labels

Comments

@WestLangley
Copy link
Collaborator

The three.js convention is to create single instances and reuse them.

@Mugen87 Mugen87 added the Addons label Nov 12, 2021
@mrdoob
Copy link
Owner

mrdoob commented Nov 15, 2021

Are there any that happen every frame?

@WestLangley
Copy link
Collaborator Author

Every time the mouse moves.

If the library is consistent in avoiding that pattern, then you don't have to worry about it.

@WestLangley
Copy link
Collaborator Author

There is still more to be done here...

@mrdoob mrdoob modified the milestones: r138, r139 Feb 23, 2022
@mrdoob mrdoob modified the milestones: r139, r140 Mar 24, 2022
@mrdoob mrdoob reopened this Apr 6, 2022
@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob modified the milestones: r141, r142 May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@WestLangley
Copy link
Collaborator Author

There is still more to be done here... anyone interested?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 14, 2022

I've looked into this issue multiple times but I'm afraid it can only be solved by rewriting major parts of the class. The problem is that certain methods like calculateRotationAxis() or getCursorNDC() return new objects. And the calling code relies on this pattern and assumes to get new objects. Besides, many intermediate objects are not placed in the module scope but are properties of ArcballControls. Every time I have tried to refactor all of this, things started to break. Besides, ArcballControls has by far the biggest code base of all control classes. That makes this task even more time consuming.

I'm not sure its worth to invest a lot of time in this class unless a user reports performance issues.

@WestLangley
Copy link
Collaborator Author

Every time I have tried to refactor all of this, things started to break.

This is red flag, then. The OP is not responding, and in addition, four devs have demonstrated little interest in pursuing it further.

If this code is not going to be maintained, it should be hosted elsewhere by someone willing to take responsibility for it.

@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob removed this from the r144 milestone Aug 31, 2022
@mrdoob mrdoob added this to the r145 milestone Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@mrdoob mrdoob modified the milestones: r147, r148 Nov 30, 2022
@mrdoob mrdoob modified the milestones: r148, r149 Dec 22, 2022
@mrdoob mrdoob modified the milestones: r149, r150 Jan 26, 2023
@mrdoob mrdoob modified the milestones: r150, r151 Feb 23, 2023
@mrdoob mrdoob modified the milestones: r151, r152 Mar 30, 2023
@mrdoob mrdoob modified the milestones: r152, r153 Apr 27, 2023
@WestLangley WestLangley removed this from the r153 milestone May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants