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

DragControls: Refactor API. #29079

Merged
merged 6 commits into from
Aug 8, 2024
Merged

DragControls: Refactor API. #29079

merged 6 commits into from
Aug 8, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Aug 7, 2024

Related issue: #29072, #20575

Description

This PR refactors the API of DragControls.

  • THREE.Controls is introduced which defines a basic control interface. DragControls is now derived from this class.
  • Side effects in the ctor are moved into a connect() method.
  • domElement is now optional. If required, an instance of DragControls can now be created without a DOM element. When the element is set at a later point, a manual call of connect() configures the event listeners.

@Mugen87 Mugen87 added this to the r168 milestone Aug 8, 2024
@Mugen87 Mugen87 merged commit b9a14e2 into mrdoob:dev Aug 8, 2024
9 of 10 checks passed
@puxiao
Copy link
Contributor

puxiao commented Aug 12, 2024

Do you think it is appropriate to add saveState():xxx, setState(xxx), and reset() to Controls?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 12, 2024

Not all controls support these methods so I would revisit this after all classes has been updated.

If we add these methods to the Controls interface, we should make sure all derived classes implement them. They are useful in any event.

@mrdoob
Copy link
Owner

mrdoob commented Aug 14, 2024

I think we should move Controls.js to src. Maybe src/extras?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 14, 2024

Sounds good!


}

connect() {}
Copy link
Owner

Choose a reason for hiding this comment

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

@Mugen87 What do you think about being able to pass the dom element here?

That way the code could look like this:

const controls = new OrbitControls( camera );
controls.connect( renderer.domElement );

I guess we'll need to do a bit of management though:

connect( element ) {

    if ( this.domElement !== element ) {

        this.disconnect();

    }

    this.domElement = element;

}

Copy link
Collaborator Author

@Mugen87 Mugen87 Sep 9, 2024

Choose a reason for hiding this comment

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

As a convenient feature, I guess it's fine.

That said, domElement is public so devs can basically achieve the same without further changes to the control classes. However, the additional management code in connect() could prevent mistakes when devs want to exchange the DOM element (not sure how common this use case is though).


class Controls extends EventDispatcher {

constructor( object, domElement ) {
Copy link
Owner

Choose a reason for hiding this comment

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

@Mugen87

constructor( object, domElement = null ) maybe?

Right now it's undefined by default but subclasses set it to null...

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.

3 participants