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

Implement Zones inheriting colors from parent Zones as an option. #885

Closed
wants to merge 1 commit into from

Conversation

mcourteaux
Copy link
Contributor

Option for coloring zones without custom color by inheriting it from a parent that has a custom color. If inherited, the color is made slightly darker to distinguish the "sources" of the colors and the ones inheriting from each other.

Option saved to ini file.

Performance considerations:

  • parents only fetched when feature enabled and zone-to-be-drawn does not have a custom color.
  • parent array is alloca()'d

Screenshot without:

image

With:

image

@mcourteaux
Copy link
Contributor Author

Closes #884

@mcourteaux
Copy link
Contributor Author

Fixed the capitalization in the screenshot: Inherit Colors -> Inherit colors.
Perhaps I should even change it to Inherit custom?

@mcourteaux
Copy link
Contributor Author

Another example of this in my jobsystem. I used this to make every job get a different color:

image

@wolfpld
Copy link
Owner

wolfpld commented Sep 13, 2024

Fixed the capitalization in the screenshot: Inherit Colors -> Inherit colors.
Perhaps I should even change it to Inherit custom?

I would name it "Inherit parent colors".

I would also rename GetZoneParentsUntilTopLevel to GetAllZoneParents, with an assert that the final depth is the depth passed as an parameter.

One more thought: the zone timeline drawing routines are already passing through all the parents. Maybe it would be simpler to just pass the user zone color down to children right there, so that no parent lookup is needed at all?

@mcourteaux
Copy link
Contributor Author

One more thought: the zone timeline drawing routines are already passing through all the parents. Maybe it would be simpler to just pass the user zone color down to children right there, so that no parent lookup is needed at all?

I could do that, but that would require adding a uint32_t custom_color field to struct TimelineDraw, which you've been trying really hard to make it small it seems.

I would name it "Inherit parent colors".

I would also rename GetZoneParentsUntilTopLevel to GetAllZoneParents, with an assert that the final depth is the depth passed as an parameter.

Will do.

@wolfpld
Copy link
Owner

wolfpld commented Sep 13, 2024

I could do that, but that would require adding a uint32_t custom_color field to struct TimelineDraw, which you've been trying really hard to make it small it seems.

The amount of timeline items to draw turns out to be surprisingly low, so it's not a concern.

@mcourteaux
Copy link
Contributor Author

It's more annoying to implement than I expected. The utility function View::GetZoneColor() is private to View. So from within TimelineItemThread::PreprocessZoneLevel(), I can't use that function to get the color there and populate the TimelineDraw::customColor field.

@wolfpld
Copy link
Owner

wolfpld commented Sep 13, 2024

Just change it to public.

@mcourteaux
Copy link
Contributor Author

Changed it and reverted old changes (rewrote git history).

This new approach also allowed me to implement correctly the color for the Folded zones:

image

@mcourteaux
Copy link
Contributor Author

Done 😄

@wolfpld
Copy link
Owner

wolfpld commented Sep 13, 2024

I forgot that zone colors may come from two different sources, so I took a deep dive to understand the code and b359936 is the result. Please take a look if everything there works as you'd expect.

@wolfpld wolfpld closed this Sep 13, 2024
@mcourteaux
Copy link
Contributor Author

mcourteaux commented Sep 14, 2024

You introduced a bug somehow, I believe. Neighboring zones adopt the color as well, randomly:

image

The zone pointed at does not have any color. Fix in #886 .

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.

2 participants