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

Reduce draw calls #6707

Merged
merged 94 commits into from
Jun 6, 2023
Merged

Reduce draw calls #6707

merged 94 commits into from
Jun 6, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented May 16, 2023

Pull Request Description

Implements #6544 (eliminates 10/42 of the constantly-displayed draw calls).
Fixes #6717. Improves startup CPU time by 5% (250ms, loading Orders on my dev box).

Important Notes

  • Edges: New implementation uses only Rectangle under most conditions.
  • Node and action area: Replace some shapes with Rectangle.
  • List view: Replace some shapes with Rectangle.
  • Display object hierarchy: The lowest-level shape instance types no longer have their own display objects.
  • Includes initial support for using Rectangle to display triangles.

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 self-assigned this May 16, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label May 16, 2023
@kazcw
Copy link
Contributor Author

kazcw commented May 16, 2023

I'll be testing this some more in the morning (with a Book Club), but it's ready for review.

Copy link
Contributor

@vitvakatu vitvakatu left a comment

Choose a reason for hiding this comment

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

I can also see some changes to the list view. Please mention them in the PR description so QA reviewers know what to check carefully.

app/gui/view/graph-editor/src/component/edge.rs Outdated Show resolved Hide resolved
app/gui/view/graph-editor/src/component/edge.rs Outdated Show resolved Hide resolved
Comment on lines 246 to 262
source_width: default(),
source_height: default(),
target_position: default(),
target_attached: default(),
color: default(),
sections: default(),
split_arc: default(),
hover_sections: default(),
corner_points: default(),
target_attachment: default(),
hover_position: default(),
disabled: default(),
previous_target: default(),
previous_is_hoverable: default(),
previous_hover_split: default(),
previous_color: default(),
previous_target_attached: default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if using ..default() is reasonable here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish I could do it that way, but I can't impl Default for EdgeModel because it must hold a Scene, so I can't default-construct an EdgeModel object to use for field-update syntax.

Copy link
Member

Choose a reason for hiding this comment

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

We could split this into two structs and deref to the inner struct, making the inner struct Default. I'm not sure though if we want it. Up to you :)

$(output.push(&self.$field);)*
output
}
fn source_x_range(&self) -> f32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is source_x_range? I'd expect Range<f32> as the return type for anything with range in the name.

@kazcw
Copy link
Contributor Author

kazcw commented May 16, 2023

There's a flicker when the mouse enters/leaves the arc area of a corner, caused by an existing bug. Issue: #6717

@kazcw
Copy link
Contributor Author

kazcw commented Jun 1, 2023

Updated demo; the old edges are in red:

vokoscreenNG-2023-06-01_16-37-03.webm

Known differences:

  • At the beginning of the video, you can see the radius is different in some positions when the target is very close to the source. The old edge snaps abruptly between a sharp corner and a smoother angle. The new edge interpolates between these extremes, avoiding a sudden transition in this case.
  • Near the end of the video, when the target is slightly higher than the source, and barely to the side of the source, the old edges form a shark fin shape, with two different radii side by side. The new edges maintain symmetry in this case.

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Looks fantastic! Let's remove the old edge impl and let's merge it! Amazing job with fixing the corner cases where the old edges misbehaved!

Comment on lines 32 to 40
//! # Partial edges
//!
//! Corners are sufficient to draw any complete edge; however, in order to split an edge into a
//! focused portion and an unfocused portion at an arbitrary location based on the mouse position,
//! we need to subdivide one of the corners of the edge.
//!
//! |\
//! | 3
//! /
Copy link
Member

Choose a reason for hiding this comment

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

these docs are beautiful ❤️

@kazcw kazcw mentioned this pull request Jun 2, 2023
5 tasks
@kazcw kazcw requested a review from mwu-tow as a code owner June 2, 2023 15:19
@kazcw kazcw linked an issue Jun 2, 2023 that may be closed by this pull request
@vitvakatu
Copy link
Contributor

QA: passed 📗

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Jun 6, 2023
@mergify mergify bot merged commit ff51347 into develop Jun 6, 2023
@mergify mergify bot deleted the wip/kw/fast-edges branch June 6, 2023 17:09
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.

New shape instance render latency Lowering draw calls
3 participants