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

feat(fe2): Update Section Box controls to include visibility #3333

Merged
merged 23 commits into from
Oct 25, 2024

Conversation

andrewwallacespeckle
Copy link
Contributor

@andrewwallacespeckle andrewwallacespeckle commented Oct 17, 2024

image

(User request)

Allow user to hide the section box including the gizmo after making the "cut".

Now: Section box is reset when you deselect in the section box tool.

Future: Save the current cut when clicking away from the section box tool. Instead show the "Reset filters" button in the bottom so users can reset the model from there.

This is already supported per the viewer docs.

Copy link

linear bot commented Oct 17, 2024

@@ -180,6 +180,10 @@ export class LegacyViewer extends Viewer {
this.sections.enabled = true
}

public setSectionBoxVisibility(visible: boolean) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexandruPopovici I made this change to test my frontend work, it works so I've left it in, but can you check incase I've done something wrong here?

One problem is that when the frontend updates the section box visibility, I need to slightly move the camera for the change to be visible. EG. Turn off section box visibility. Section box will remain, until I slightly move the camera. Is this something that should be fixed in the viewer? I can probably fix in frontend, but it would be a bit hacky.

Don't think there's any great urgency in this task.

Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewwallacespeckle
The viewer employs on-demand rendering. This means that it is not constantly rendering, but rather it only does so when it needs to, or when it's told to. We're doing this mostly for mobile platform's sake where rendering each frame would drain battery pointlessly.
That's why whenever the client does something that needs to trigger a render, it needs to call requestRender, otherwise no change can be seen since there is no rendering happening. Moving the camera triggers a render request implicitly, that's why you were seeing that behavior.

I've added the calls to requestRender in the places that were missing it. Also, I've added a better approach on showing/hiding the section box that does not go through the LegacyViewer. We shouldn't add functionality in there, we actually need to retire it.

Let me know if you're having other issues with this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexandruPopovici, that's working perfectly! Thanks for the explanation too, I'll know better for next time.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

@andrewwallacespeckle andrewwallacespeckle marked this pull request as ready for review October 18, 2024 10:12
@andrewwallacespeckle
Copy link
Contributor Author

I have made this so the toggle in the left hand menu toggle visibility. This allows the user to toggle it using shift+b.

On load, section box is both disabled, and invisible. On first click, it is enabled and set as visible. This will also trigger the "Reset Section Box" button.

Any further clicks to the sidebar button will simply toggle visibility. A click of reset will return everything to it's original state.

@benjaminvo Can you check the UI and see if you are happy with this implementation? If so I'll get it in today and reply to the user on the forum who requested it.

Mikehrn
Mikehrn previously approved these changes Oct 18, 2024
@benjaminvo
Copy link
Contributor

@andrewwallacespeckle I like it! Only issue is that the "Reset section box" button appears as soon as you enter the section box tool. It should ideally only appear if the section box has been edited.

If this somehow shows to be difficult you can also just merge this now and create a follow up PR.

Copy link
Contributor

📸 Preview service has generated an image.

@andrewwallacespeckle
Copy link
Contributor Author

@andrewwallacespeckle I like it! Only issue is that the "Reset section box" button appears as soon as you enter the section box tool. It should ideally only appear if the section box has been edited.

If this somehow shows to be difficult you can also just merge this now and create a follow up PR.

This change has been made and it is now ready for a final review

Copy link
Contributor

📸 Preview service has generated an image.

@AlexandruPopovici
Copy link
Contributor

But as you can see in the Loom at 1:20 we don't show outlines if you view a comment that was made on sectioned model

@benjaminvo Yeah, outlines should be visible there

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

@benjaminvo
Copy link
Contributor

@andrewwallacespeckle I just tested – is it just me or does the section box now reset when you toggle the tool?

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

Copy link
Contributor

📸 Preview service has generated an image.

@andrewwallacespeckle andrewwallacespeckle merged commit 678c753 into main Oct 25, 2024
25 of 27 checks passed
@andrewwallacespeckle andrewwallacespeckle deleted the andrew/web-1962-hide-section-box branch October 25, 2024 09:46
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.

5 participants