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

Show/hide thread lanes and make them collapsible #97

Merged
merged 2 commits into from
Oct 4, 2022

Conversation

TimonPost
Copy link
Member

@TimonPost TimonPost commented Sep 28, 2022

Checkboxes for hiding and showing puffin flame graph threads.

The collection is stored in Options as it is nice for it to be saved upon application closing. Also used index map such that order is preserved, we get fast insertion and reading, and only get unique thread entries are shown in thje checkbox list. By storing it in Options we do generate an index map that gets extended longer and longer with time as all threads from all profiled puffin files/applications eventually end up in there. Tho as we still iterate through the currently selected frame threads those other threads do not show up in our checkbox list. Think its not to big of a deal.

New option to disable thread lanes:

image

image

Collapsable threads:

image

@TimonPost TimonPost requested review from NiklasNummelin and removed request for emilk and VZout September 28, 2022 14:25
@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 29, 2022

On the screen the thread list seems to take a significant vertical place.
It is possible to add collapse possibility ? Like Frames display.

@TimonPost
Copy link
Member Author

TimonPost commented Sep 29, 2022

Maybe like a collapsable settings bar? All settings could be under this bar in a panel.

@TimonPost
Copy link
Member Author

TimonPost commented Sep 29, 2022

Looks nice to me with a collapsable widigty

image

image

@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 29, 2022

I think it's good to have some settings accessible without need to unfold.
I think of Help ? sort setting ? Scope Filter ?
Show thread setting could be at right instead to be at left ?

As alternative, I would like to be able to fold/unfold thread display directly in flamegraph.

@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 29, 2022

To illustrate (quick proof of concept):
image
And :
image

@TimonPost
Copy link
Member Author

TimonPost commented Sep 29, 2022

like that idea as well! Will give it a try and see the possibilities there

@TimonPost
Copy link
Member Author

Tho need to find a way on how to persist the settings. Expect it to be a bit more tricky doing it that way.

@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 29, 2022

I don't know how persistent settings work in puffin (viewer ?).
But I used the same options.show_flamegraph_threads as you in my test.

@TimonPost
Copy link
Member Author

TimonPost commented Sep 30, 2022

@gwen-lg you seem to sugegest you somewhat implement it? I have it somewhat working, only missing arrows, but the whole thread section is not using egui::Ui but rather uses Painter to draw the UI. Collaps headers as it is provided by egui can only be used it seems with a proper instance of egui::UI.

@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 30, 2022

Yes, I was thinking yesterday that use Painter instead of egui::Ui have disadvantages, and perhaps isn't the best solution.

But with the existing, I have copy and adapt the code of the CollapsingHeader display. This code directly construct the arrow to display, and eventually rotate it.
Original code is in fn paint_default_icon in collapsing_header.rs.
On my side I hacked paint_thread_info like this (PS: it's not clean code, it's probably need to be improved) :

fn paint_thread_info(info: &Info, thread: &ThreadInfo, show: &mut bool, pos: Pos2) {
    let pos_arrow = pos2(pos.x + 2., pos.y + 2.);
    let rect_arrow = Rect::from_min_size(pos_arrow, vec2(8., 8.));

    // Draw a pointy triangle arrow:
    let rect_arrow = Rect::from_center_size(
        rect_arrow.center(),
        vec2(rect_arrow.width(), rect_arrow.height()) * 0.75,
    );

    let mut points = vec![
        rect_arrow.left_top(),
        rect_arrow.right_top(),
        rect_arrow.center_bottom(),
    ];
    if !*show {
        use std::f32::consts::TAU;
        let rotation = emath::Rot2::from_angle(-TAU / 4.0); //90°
        for p in &mut points {
            *p = rect_arrow.center() + rotation * (*p - rect_arrow.center());
        }
    }

    let galley = info.ctx.fonts().layout_no_wrap(
        thread.name.clone(),
        info.font_id.clone(),
        Rgba::from_white_alpha(0.9).into(),
    );
    let text_pos = pos2(pos.x + 14., pos.y);
    let rect_text = Rect::from_min_size(text_pos, galley.size());

    let rect = Rect::from_min_max(rect_arrow.min, rect_text.max);
    info.painter
        .rect_filled(rect.expand(2.0), 0.0, Rgba::from_black_alpha(0.5));
    info.painter.add(Shape::closed_line(
        points,
        Stroke::new(1., Rgba::from_white_alpha(0.9)),
    ));
    info.painter.galley(rect_text.min, galley);

    let is_hovered = if let Some(mouse_pos) = info.response.hover_pos() {
        rect.contains(mouse_pos)
    } else {
        false
    };

    if is_hovered && info.response.clicked() {
        *show = !(*show);
    }
}

An wanted part of my version is than arrow icon and thread name are a unique block, and all than block is clickable.
As example of improvement : I think it's should probably better than paint_thread_info return the information than the thread name was clicked, and the update of show variable updated in the caller method.

@gwen-lg
Copy link
Contributor

gwen-lg commented Sep 30, 2022

My test, based on your work is also available here : https://github.com/gwen-lg/puffin/tree/hide-threads-flamegraph_test

@TimonPost
Copy link
Member Author

TimonPost commented Oct 1, 2022

Alright. I went for the arrows from here: https://docs.rs/egui/latest/egui/special_emojis/index.html, instead of painting them in code. Most of the UI is similar to before, but I split the settings panel into two columns.

I now have:

  • Option to collapse threads.
  • Option to hide threads completely.

Collapsible threads

When one clicks on thread name, the thread lane is collapsedsed.

image

image

Hiding possibility

When the checkbox is selected, the thread is completely hidden.

image
image

puffin_egui/src/flamegraph.rs Outdated Show resolved Hide resolved
@gwen-lg
Copy link
Contributor

gwen-lg commented Oct 1, 2022

@TimonPost : can you "clean" commits ?
I found it easier to do code reviews, and read history.
By squash fmt, clippy and rename commits in corresponding commit. Git-Tools-Rewriting-History
I use the git interactive rebase a lot, this help to have cleaner repot history and help future work (when you need to read history)

Note: if you use VSCode/VSCodium, you kown the "editor.formatOnSave" settings option ?
https://code.visualstudio.com/docs/getstarted/settings
This avoid to forget cargo fmt before commit/push.

@TimonPost TimonPost force-pushed the timon/hide-threads-flamegraph branch 2 times, most recently from 632a560 to 5dc144b Compare October 1, 2022 15:03
@TimonPost
Copy link
Member Author

TimonPost commented Oct 1, 2022

Maybe can enable that config setting project-wide, but will def. do it in my own editor as well. It's indeed slightly annoying to find out that after CI ran fmt wasn't run. I squashed merged commits and rebased the branch, I dont really want to bother cleaning every single commit up as the CI will do a squash merge anyways.

@TimonPost TimonPost changed the title Show/hide flamegraph puffin threads Show/hide threads and make them collapsible Oct 1, 2022
@TimonPost TimonPost changed the title Show/hide threads and make them collapsible Show/hide thread lanes and make them collapsible Oct 1, 2022
@repi
Copy link
Contributor

repi commented Oct 3, 2022

no need to clean up the commits @TimonPost , we squash merge

@TimonPost TimonPost force-pushed the timon/hide-threads-flamegraph branch from 5dc144b to e8e9e7d Compare October 4, 2022 09:14
@TimonPost TimonPost merged commit cb34d55 into main Oct 4, 2022
@TimonPost TimonPost deleted the timon/hide-threads-flamegraph branch October 4, 2022 09:38
@emilk
Copy link
Collaborator

emilk commented Oct 4, 2022

nice! what do you think about making a new release?

puffin_http could do with a patch release due to #98,
and puffin_egui and puffin_viewer a new minor release due to this PR

@TimonPost
Copy link
Member Author

@emilk yes that sounds like a good idea and is in the queue. Need to update change logs and then I can do a release. Maybe I might want to sneak in a few other changes but could be done later as well.

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