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

Pending nodes #7541

Merged
merged 4 commits into from
Aug 17, 2023
Merged

Pending nodes #7541

merged 4 commits into from
Aug 17, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Aug 10, 2023

Pull Request Description

Implement pending node status (#7435).

Design reference:
https://user-images.githubusercontent.com/3919101/257222729-838cfe84-7447-40a3-8aa1-5424de66b0b7.png

Demo:
vokoscreenNG-2023-08-09_22-29-13.webm

Important Notes

  • Reduce alpha value of node backgrounds, label widgets, and method widget icons while the engine reports "pending" state.
  • Use FRP and themes for method widgets.
  • Update node input area to use define_endpoints_2.
  • Also remove some code supporting the disused profiling mode.

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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 10, 2023
@kazcw kazcw self-assigned this Aug 10, 2023
@kazcw kazcw marked this pull request as ready for review August 10, 2023 06:52
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.

LGTM, but I'd like to have widget's code checked by @Frizi

Copy link
Contributor

@Frizi Frizi left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some minor stylistic nits.

app/gui/src/presenter/graph/state.rs Outdated Show resolved Hide resolved
app/gui/src/presenter/graph/state.rs Outdated Show resolved Hide resolved
app/gui/view/graph-editor/src/component/node.rs Outdated Show resolved Hide resolved
@kazcw kazcw linked an issue Aug 11, 2023 that may be closed by this pull request
@MichaelMauderer
Copy link
Contributor

QA: 🟡

This is not clearly defined in the reference design, but with the current implementation, I can see the edges going under the transparent nodes:

image

This looks weird and unintentional.

@kazcw
Copy link
Contributor Author

kazcw commented Aug 17, 2023

QA: yellow_circle

This is not clearly defined in the reference design, but with the current implementation, I can see the edges going under the transparent nodes:

image

This looks weird and unintentional.

image

I've adjusted this at the input port (target attachment) ends, however I can't make the output edges look right by changing their end points, since a flat-ended line can't meet a rounded node exactly. Our current rendering system cannot reconcile performance (minimizing shader count) with transparency. When we eventually switch to Z-buffer compositing, we can draw this correctly.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Aug 17, 2023
@mergify mergify bot merged commit d15b3db into develop Aug 17, 2023
25 checks passed
@mergify mergify bot deleted the wip/kw/pending-nodes branch August 17, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pending node status
4 participants