addressing code review

This commit is contained in:
Marcos Slomp
2026-09-21 08:27:00 -07:00
parent 9fb9bd7574
commit 1079294b95
5 changed files with 69 additions and 69 deletions

View File

@@ -55,20 +55,14 @@ void TimelineController::UpdateCenterItem( int pinnedTop )
// Pinned items are fixed to the viewport and excluded from the scrolling
// flow, so centering only considers the normal items in the middle band.
const void* firstNormalKey = nullptr;
const void* lastNormalKey = nullptr;
for( auto& item : m_items )
{
if( item->IsPinned() ) continue;
if( !firstNormalKey ) firstNormalKey = item->GetKey();
lastNormalKey = item->GetKey();
}
if( m_normalItems.empty() ) return;
const void* firstNormalKey = m_normalItems.front()->GetKey();
const void* lastNormalKey = m_normalItems.back()->GetKey();
int yBegin = 0;
int yEnd = pinnedTop;
for( auto& item : m_items )
for( auto& item : m_normalItems )
{
if( item->IsPinned() ) continue;
m_centerItemkey = item->GetKey();
yBegin = yEnd;
yEnd += item->GetHeight();
@@ -96,9 +90,8 @@ std::optional<int> TimelineController::CalculateScrollPosition( int pinnedTop )
int yBegin = 0;
int yEnd = pinnedTop;
for( auto& item : m_items )
for( auto& item : m_normalItems )
{
if( item->IsPinned() ) continue;
yBegin = yEnd;
yEnd += item->GetHeight();
@@ -116,16 +109,23 @@ std::optional<int> TimelineController::CalculateScrollPosition( int pinnedTop )
void TimelineController::End( double pxns, const ImVec2& wpos, bool hover, bool vcenter, float yMin, float yMax )
{
// Snapshot the pin classification once per frame before any Draw runs.
// Toggling a pin flips its state mid-frame and since re-checking it in
// the draw passes would draw the item twice (normal and band passes),
// the grouping here defers the change to the next frame.
// GetHeight() is 0 on the first frame, so the scroll extent would be shorter
// if a track was pinned at load, but would "self-correct" in the next frame.
// (Also, nothing is pinned at load anyway.)
// if a track was pinned at load, but self-corrects next frame (and nothing is
// pinned at load anyway).
m_normalItems.clear();
m_pinnedTopItems.clear();
m_pinnedBottomItems.clear();
int pinnedTop = 0;
int pinnedBottom = 0;
for( auto& item : m_items )
{
if( !item->IsPinned() ) continue;
if( item->PinToBottom() ) pinnedBottom += item->GetHeight();
else pinnedTop += item->GetHeight();
if( !item->IsPinned() ) m_normalItems.push_back( item );
else if( item->PinToBottom() ) { m_pinnedBottomItems.push_back( item ); pinnedBottom += item->GetHeight(); }
else { m_pinnedTopItems.push_back( item ); pinnedTop += item->GetHeight(); }
}
auto shouldUpdateCenterItem = [&] () {
@@ -171,75 +171,72 @@ void TimelineController::End( double pxns, const ImVec2& wpos, bool hover, bool
// would cover top ones while leaving them clickable underneath.
const int bottomStart = std::max( curScrollY + pinnedTop, curScrollY + windowHeight - pinnedBottom );
// Pinned items add the scroll offset so they stay fixed as content scrolls.
auto itemYOffset = [&] ( TimelineItem* item, int topRun, int normalRun, int bottomRun ) -> int {
if( !item->IsPinned() ) return pinnedTop + normalRun;
return item->PinToBottom() ? bottomStart + bottomRun : curScrollY + topRun;
};
// Screen-space band edges, shared by culling, the mouse tests, and the fills.
const float topBandBegin = wpos.y + curScrollY;
const float topBandEnd = topBandBegin + pinnedTop;
const float bottomBandBegin = wpos.y + bottomStart;
const float bottomBandEnd = topBandBegin + windowHeight;
TimelineContext ctxNormal = ctx;
if( pinnedTop > 0 ) ctxNormal.yMin = std::max<float>( ctx.yMin, topBandEnd );
if( pinnedBottom > 0 ) ctxNormal.yMax = std::min<float>( ctx.yMax, bottomBandBegin );
const auto mouseY = ImGui::GetMousePos().y;
const bool mouseInTopBand = pinnedTop > 0 && mouseY >= topBandBegin && mouseY < topBandEnd;
const bool mouseInBottomBand = pinnedBottom > 0 && mouseY >= bottomBandBegin && mouseY < bottomBandEnd;
ctxNormal.hover = ctx.hover && !mouseInTopBand && !mouseInBottomBand;
// Preprocess runs before any Draw, so live pin state is stable here. Normal
// items are culled to the middle band and pinned items are always on screen.
int topOffset = 0, normalOffset = 0, bottomOffset = 0;
for( auto& item : m_items )
{
const int off = itemYOffset( item, topOffset, normalOffset, bottomOffset );
const bool pinned = item->IsPinned();
const bool toBottom = pinned && item->PinToBottom();
const int off = !pinned ? pinnedTop + normalOffset : toBottom ? bottomStart + bottomOffset : curScrollY + topOffset;
if( item->WantPreprocess() && item->IsVisible() )
{
const auto yPos = wpos.y + off;
const bool visible = item->IsPinned() || m_firstFrame || ( yPos < yMax && yPos + item->GetHeight() >= yMin );
const bool visible = pinned || m_firstFrame || ( yPos < ctxNormal.yMax && yPos + item->GetHeight() >= ctxNormal.yMin );
item->Preprocess( ctx, m_td, visible, yPos );
}
const int h = m_firstFrame ? 0 : item->GetHeight();
if( !item->IsPinned() ) normalOffset += h;
else if( item->PinToBottom() ) bottomOffset += h;
if( !pinned ) normalOffset += h;
else if( toBottom ) bottomOffset += h;
else topOffset += h;
}
m_td.Sync();
auto draw = ImGui::GetWindowDrawList();
// Matches the timeline background in normal builds: the root-window build
// overrides WindowBg per-window, so the fill can be a slightly off shade there.
const auto bgColor = ImGui::GetColorU32( ImGuiCol_WindowBg );
// Keep tracks scrolling under a pinned band inert (cull their headers to the
// middle region and drop hover while the mouse is over a band).
TimelineContext ctxNormal = ctx;
if( pinnedTop > 0 ) ctxNormal.yMin = std::max<float>( ctx.yMin, wpos.y + curScrollY + pinnedTop );
if( pinnedBottom > 0 ) ctxNormal.yMax = std::min<float>( ctx.yMax, wpos.y + bottomStart );
const auto mouseY = ImGui::GetMousePos().y;
const bool mouseInTopBand = pinnedTop > 0 && mouseY >= wpos.y + curScrollY && mouseY < wpos.y + curScrollY + pinnedTop;
const bool mouseInBottomBand = pinnedBottom > 0 && mouseY >= wpos.y + bottomStart && mouseY < wpos.y + curScrollY + windowHeight;
ctxNormal.hover = ctx.hover && !mouseInTopBand && !mouseInBottomBand;
int normalRunning = 0;
for( auto& item : m_items )
{
if( item->IsPinned() ) continue;
auto h = item->GetHeight();
item->Draw( m_firstFrame, ctxNormal, pinnedTop + normalRunning );
if( m_firstFrame ) h = item->GetHeight();
normalRunning += h;
}
// Draws a group at (base + accumulated height) with respect to the
// first-frame height bootstrap, and returns the total height drawn.
auto drawRun = [&]( std::vector<TimelineItem*>& items, const TimelineContext& c, int base ) -> int {
int running = 0;
for( auto& item : items )
{
auto h = item->GetHeight();
item->Draw( m_firstFrame, c, base + running );
if( m_firstFrame ) h = item->GetHeight();
running += h;
}
return running;
};
if( pinnedTop > 0 ) draw->AddRectFilled( ImVec2( wpos.x, wpos.y + curScrollY ), ImVec2( wpos.x + ctx.w, wpos.y + curScrollY + pinnedTop ), bgColor );
int topRunning = 0;
for( auto& item : m_items )
{
if( !item->IsPinned() || item->PinToBottom() ) continue;
auto h = item->GetHeight();
item->Draw( m_firstFrame, ctx, curScrollY + topRunning );
if( m_firstFrame ) h = item->GetHeight();
topRunning += h;
}
const int normalRunning = drawRun( m_normalItems, ctxNormal, pinnedTop );
if( pinnedBottom > 0 ) draw->AddRectFilled( ImVec2( wpos.x, wpos.y + bottomStart ), ImVec2( wpos.x + ctx.w, wpos.y + curScrollY + windowHeight ), bgColor );
int bottomRunning = 0;
for( auto& item : m_items )
{
if( !item->IsPinned() || !item->PinToBottom() ) continue;
auto h = item->GetHeight();
item->Draw( m_firstFrame, ctx, bottomStart + bottomRunning );
if( m_firstFrame ) h = item->GetHeight();
bottomRunning += h;
}
// Opaque fills so tracks scrolling under a band do not show through. These also
// hide parent-list overlays drawn before the child.
if( pinnedTop > 0 ) draw->AddRectFilled( ImVec2( wpos.x, topBandBegin ), ImVec2( wpos.x + ctx.w, topBandEnd ), bgColor );
drawRun( m_pinnedTopItems, ctx, curScrollY );
if( pinnedBottom > 0 ) draw->AddRectFilled( ImVec2( wpos.x, bottomBandBegin ), ImVec2( wpos.x + ctx.w, bottomBandEnd ), bgColor );
drawRun( m_pinnedBottomItems, ctx, bottomStart );
int yOffset = pinnedTop + normalRunning + pinnedBottom;
// pinnedTop is a pre-Draw height, so vertical-centre compensation lags one frame
if( const auto scrollY = CalculateScrollPosition( pinnedTop ) )
{
int clampedScrollY = std::min<int>( *scrollY, std::max<int>( yOffset - ImGui::GetWindowHeight(), 0 ) );

View File

@@ -50,6 +50,9 @@ private:
std::optional<int> CalculateScrollPosition( int pinnedTop ) const;
std::vector<TimelineItem*> m_items;
std::vector<TimelineItem*> m_normalItems;
std::vector<TimelineItem*> m_pinnedTopItems;
std::vector<TimelineItem*> m_pinnedBottomItems;
unordered_flat_map<const void*, std::unique_ptr<TimelineItem>> m_itemMap;
float m_height;

View File

@@ -319,7 +319,7 @@ private:
void DrawTimeline();
void DrawSampleList( const TimelineContext& ctx, const std::vector<SamplesDraw>& drawList, const Vector<SampleData>& vec, int offset, uint64_t tid );
void DrawZoneList( const TimelineContext& ctx, const std::vector<TimelineDraw>& drawList, int offset, uint64_t tid, int maxDepth, double margin );
void DrawThreadCropper( const int depth, const uint64_t tid, const float xPos, const float yPos, const float ostep, const float cropperWidth, const bool hasCtxSwitches );
void DrawThreadCropper( const int depth, const uint64_t tid, const float xPos, const float yPos, const float ostep, const float cropperWidth, const bool hasCtxSwitches, const bool hover );
void DrawContextSwitchList( const TimelineContext& ctx, const std::vector<ContextSwitchDraw>& drawList, const Vector<ContextSwitchData>& ctxSwitch, int offset, int endOffset, bool isFiber, uint64_t tid );
int DispatchGpuZoneLevel( const Vector<short_ptr<GpuEvent>>& vec, bool hover, double pxns, int64_t nspx, const ImVec2& wpos, int offset, int depth, uint64_t thread, float yMin, float yMax, int64_t begin, int drift );
template<typename Adapter, typename V>

View File

@@ -445,7 +445,7 @@ bool View::DrawCpuData( const TimelineContext& ctx, const std::vector<CpuUsageDr
offset += sstep;
}
if( ImGui::IsMouseHoveringRect( wpos, wpos + ImVec2( w, offset ) ) && IsMouseClickReleased( ImGuiMouseButton_Left ) )
if( hover && ImGui::IsMouseHoveringRect( wpos, wpos + ImVec2( w, offset ) ) && IsMouseClickReleased( ImGuiMouseButton_Left ) )
{
if( m_drawThreadHighlight != 0 )
{

View File

@@ -99,7 +99,7 @@ void View::DrawThread( const TimelineContext& ctx, const ThreadData& thread, con
if( displayCropper )
{
ImGui::PopClipRect();
if( depth > 0 ) DrawThreadCropper( depth, thread.id, croppperPosX, yPos, ostep, cropperWidth, hasCtxSwitch );
if( depth > 0 ) DrawThreadCropper( depth, thread.id, croppperPosX, yPos, ostep, cropperWidth, hasCtxSwitch, ctx.hover );
}
}
@@ -606,7 +606,7 @@ void View::DrawZoneList( const TimelineContext& ctx, const std::vector<TimelineD
}
}
void View::DrawThreadCropper( const int depth, const uint64_t tid, const float xPos, const float yPos, const float ostep, const float cropperWidth, const bool hasCtxSwitches )
void View::DrawThreadCropper( const int depth, const uint64_t tid, const float xPos, const float yPos, const float ostep, const float cropperWidth, const bool hasCtxSwitches, const bool hover )
{
const ImVec2 mousePos = ImGui::GetMousePos();
const bool clicked = IsMouseClicked( ImGuiMouseButton_Left );
@@ -643,7 +643,7 @@ void View::DrawThreadCropper( const int depth, const uint64_t tid, const float x
const float dx = mousePos.x - center.x;
const float dy = mousePos.y - center.y;
if( dx * dx + dy * dy <= hradius * hradius )
if( hover && dx * dx + dy * dy <= hradius * hradius )
{
draw->AddCircle( center, hradius, 0xFFFFFFFF, 0, hoverCircleThickness );
const float wPosX = ImGui::GetWindowPos().x + ImGui::GetWindowContentRegionMin().x;