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.

Sign up for GitHub

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

Jump to bottom

Implement partially transparent visualizations #11582

Merged
merged 8 commits into from
Nov 25, 2024
Merged

Implement partially transparent visualizations #11582

merged 8 commits into from
Nov 25, 2024

Conversation

vitvakatu
Copy link
Contributor

Pull Request Description

Closes #11534

  • Visualizations are partially transparent by default
  • The initial z-order is undetermined at the project load
  • Node is moved on top of other if it is dragged (before it only happened if it was selected, which is not the same)
  • Changed rendering for edges slightly, to avoid visible edge ends underneath visualization. The implementation of additional offsets is rather naive, but it works.
transparent-vis.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@vitvakatu vitvakatu added the -gui label Nov 18, 2024
@vitvakatu vitvakatu self-assigned this Nov 18, 2024
@kazcw
Copy link
Contributor

kazcw commented Nov 19, 2024

The edges don't exactly meet the visualization:

imagem

We can render it correctly by enabling the maskSource for edges coming from nodes with visualizations. @Frizi brought up possibly switching from masking to clipping to improve performance; this would take some geometry, as we'd need to compute a shape whose inside includes the part of the line not to occlude.

@vitvakatu vitvakatu marked this pull request as draft November 21, 2024 09:38
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 22, 2024 •
edited
Loading

🧪 Storybook is successfully deployed!

📊 Dashboard:

@vitvakatu vitvakatu mentioned this pull request Nov 22, 2024
Open
@vitvakatu
Copy link
Contributor Author

vitvakatu commented Nov 22, 2024 •
edited
Loading

After some internal discussions and a PoC implementation, we found that any obvious fix for imperfections noted by @kazcw above ↑ will lead to a performance drop compared with the current develop.
That is unfortunate, but we will merge this PR and work on a proper solution later.

See #11622 and #11623.

@vitvakatu vitvakatu marked this pull request as ready for review November 22, 2024 08:26
@vitvakatu vitvakatu added CI: Ready to merge This PR is eligible for automatic merge CI: No changelog needed Do not require a changelog entry for this PR. and removed CI: Ready to merge This PR is eligible for automatic merge labels Nov 22, 2024
farmaazon
farmaazon approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

I would add a CHANGELOG entry - even if it's minor, it still a UX change.

app/gui/src/project-view/components/GraphEditor/GraphNode.vue Outdated
Comment on lines 295 to 297
watch(isDragged, (newVal, oldVal) => {
if (newVal !== oldVal) graph.db.moveNodeToTop(nodeId.value)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK there is no need for this check, as watch is run only when value changed.

@vitvakatu vitvakatu added CI: Ready to merge This PR is eligible for automatic merge and removed CI: No changelog needed Do not require a changelog entry for this PR. labels Nov 25, 2024
@mergify mergify bot merged commit b5f93f0 into develop Nov 25, 2024
52 of 54 checks passed
@mergify mergify bot deleted the wip/vitvakatu/opacity-in-vis-panel branch November 25, 2024 13:28
hubertp pushed a commit that referenced this pull request Nov 26, 2024
@vitvakatu @hubertp
Implement partially transparent visualizations (#11582)
05d8883 
Closes #11534

- Visualizations are partially transparent by default
- The initial z-order is undetermined at the project load
- Node is moved on top of other if it is dragged (before it only happened if it was **selected**, which is not the same)
- Changed rendering for edges slightly, to avoid visible edge ends underneath visualization. The implementation of additional offsets is rather naive, but it works.

https://github.com/user-attachments/assets/fba44816-eed9-471d-83a7-8fe6e5892477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farmaazon farmaazon farmaazon approved these changes

@Frizi Frizi Awaiting requested review from Frizi Frizi is a code owner

@kazcw kazcw Awaiting requested review from kazcw kazcw is a code owner

@AdRiley AdRiley Awaiting requested review from AdRiley AdRiley is a code owner

Assignees

@vitvakatu vitvakatu

Labels
-gui CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Graph Editor Look&Feel - Slight opacity in the visualization panel if not selected.
3 participants
@vitvakatu @kazcw @farmaazon