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

Playground: Implement a splitscreen #27784

Merged
merged 1 commit into from Apr 8, 2024
Merged

Playground: Implement a splitscreen #27784

merged 1 commit into from Apr 8, 2024

Conversation

abc013
Copy link
Contributor

@abc013 abc013 commented Feb 20, 2024

Description

Feature proposal for a (resizable) splitscreen for the playground:
image

Currently, there are two ways to display results of the node trees:

  1. One by pressing the leftmost button in the menu for a fullscreen-preview,
  2. or by "isolating" (double-clicking) a single node such that only it and the result is shown.

Something I found annoying during my experiments is that often times, I wanted to see the results when plugging various nodes around for experimentation. The first display option requires an additional click/hotkey (and a little do-after because of the node-pop-up animation when the editor opens back up). The second option only allows to show a single node. A splitscreen allows to see the results of plugging instantly.

I am open for suggestions and discussion :)

playground/NodeEditor.js Fixed Show fixed Hide fixed
playground/NodeEditor.js Fixed Show fixed Hide fixed
@sunag
Copy link
Collaborator

sunag commented Feb 22, 2024

  • There appears to be a problem when the window is resized.
  • When I click on preview it doesn't seem to return to the previous state of the split screen.

@abc013
Copy link
Contributor Author

abc013 commented Feb 26, 2024

There appears to be a problem when the window is resized.

Fixed. I had to wrap the renderer DOM element into another div for this, let me know if there is a better way to do this.

When I click on preview it doesn't seem to return to the previous state of the split screen.

Fixed, it does now return to the original view as demanded.

@abc013
Copy link
Contributor Author

abc013 commented Feb 27, 2024

The DeepScan check fails on an issue that seems to be totally unrelated to this PR - or am I overlooking something?

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 27, 2024

A rebase would solve the issue. But yes, it's unrelated to your changes.

const container = document.createElement( 'div' );
container.className = 'splitview';

document.body.appendChild( container );
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use this.domElement instead of document.body here and in others appends? This would make the editor more flexible if we added it embed in editor for exemple. I think we can do the same thing with the classes, for example: flow splitview intead of just splitview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.domElement is null in the context of the script (which is directly embedded into the html). Thus, I am not sure how to implement that, can you give me a hint?

@sunag
Copy link
Collaborator

sunag commented Mar 5, 2024

It's looking really cool, I think it would be the last details I saw.

@sunag sunag added this to the r163 milestone Mar 5, 2024
@@ -297,7 +339,7 @@ export class NodeEditor extends THREE.EventDispatcher {

previewMenu.add( editorButton );

this.domElement.append( menu.dom );
document.body.appendChild(menu.dom);
Copy link
Collaborator

@sunag sunag Mar 15, 2024

Choose a reason for hiding this comment

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

We need to move the menus to domElement in order to maintain the consistency of the hierarchy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you can follow the name style could be better intead of div, could remove some redundant classes name too. Something like this:

image

node-editor is domElement in this case

  • node-editor
    • f-preview ( instead of splitview )
    • flow
    • f-menu

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.
image
f-menu still lives in flow, because otherwise i would have to keep track of when the editor gets added in order to then add f-menu to the domElement parent.

is it fine like that?

@mrdoob mrdoob modified the milestones: r163, r164 Mar 29, 2024
@sunag sunag merged commit 066b6f7 into mrdoob:dev Apr 8, 2024
11 checks passed
@abc013 abc013 deleted the splitview branch April 16, 2024 13:18
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.

None yet

4 participants