[draft] Prototype for better `TLine::ExecuteEvent` by linev · Pull Request #21883 · root-project/root

linev · 2026-04-10T11:04:58Z

Interactive objects moving require to have some temporary variables stored.
Up to now many static variables were defined in TLine::ExecuteEvent which
were reused by consequent methods calls.

There are many disadvantages of such approach:

there is no guarantee from ROOT that always same TLine object instance called
values in static members remain and can be by mistake used by other objects
there is no chance to handle multiple gui event loops from different threads

Trying to make different approach when "interactive" object preserved as
transient member of the class with std::unique_ptr<>. It solves all mentioned
problems. The only disadvantage - memory consumption for each TLine
object increased by 8 bytes.

If one agrees on using of such approach - one can adjust other ROOT classes making them
more robust and moving us forward to graphics multithreading

Use PaintArrowNDC in the TArrow::Paint Use attributes ModifyOn methods Shorten options handling code

Interactive objects moving require to have some temporary variables stored. Up to now many static variables were defined in TLine::ExecuteEvent which were reused by consequent methods calls. There are many disadvantages of such approach: - there is no guarantee from ROOT that always same TLine object instance called - values in static members remain and can be by mistake used by other objects - there is no chance to handle multiple gui event loops from different threads Trying to make different approach when "interactive" object preserved as transient member of the class with std::unique_ptr<>. It let solves all of mentioned problems. The only disadvantage - memory consumption for each TLine object increased by 8 bytes.

silverweed · 2026-04-10T11:19:43Z

graf2d/graf/src/TLine.cxx

+/// Line default constructor.
+/// Defined here to correctly instantiate fInteractive member
+
+TLine::TLine()


Can be = default?

Never for TObject-based classes

but does it need to be defined in this class at all?

Yes, std::unique_ptr<> need to be initialized for class which only described in CXX file

silverweed · 2026-04-10T11:20:11Z

graf2d/graf/src/TLine.cxx

+   Int_t px1, px2, py1, py2, pxold, pyold;
+   Double_t oldX1, oldY1, oldX2, oldY2;
+   Bool_t hasOld = kFALSE;
+   Int_t selectPoint = 0;


Why are hasOld and selectPoint default-initialized but the other fields aren't?

While other depends on these two

silverweed · 2026-04-10T11:22:01Z

graf2d/graf/src/TLine.cxx

-   Bool_t opaque  = parent->OpaqueMoving();
+   ROOT::Internal::TLineInteractive *i;
+
+   auto get_interactive = [this, &i](bool force) -> bool {


Should be called getInteractive. Also I would rename force to createIfNull or something like that .

Although, I'm not sure this lambda should exist at all: why not simply embed the fields of TLineInteractive in TLine? (see comment above)

why not simply embed the fields of TLineInteractive in TLine? (see comment above)

Because then each instance of TLine will get many extra fields which never used.
So one tries to decrease memory consumption by such technique

How many such instances do we expect in the worst-case scenario? With 64 extra bytes per instance, you'd have to have more than 16k instances before you waste 1MiB of memory

TLine is very basic object and appears everywhere. But just double or triple size just of unused data? I really do not like this.

Sure, but, for one I'm not sure why we need to store oldX* as double (float precision should be more than enough for NDC coordinates);
also, I'm not sure I understand it correctly: do we ever need more than 1 instance of this data per thread at once? Or is this data only relevant while interacting with the line?
Because if that's the case I'd argue it should only live during the interaction itself (e.g. create a thread_local instance when interaction start, destroy it when interaction end).

This can be different approach.
Maybe it is even better.

silverweed · 2026-04-10T11:23:26Z

graf2d/graf/src/TLine.cxx

+      if (!fInteractive && force)
+         fInteractive = std::make_unique<ROOT::Internal::TLineInteractive>();
+      i = fInteractive.get();
+      return i != nullptr;


Instead of setting i and returning a bool, can't this directly return i? I don't like that the captured variable gets set as a side effect: it's very hard to track (since it's not even a function out parameter). I would instead do:

if (auto lineInteractive = getInteractive(false)) { // ... }

or something along these lines

By the way, i is a very bad name imho; if you want a short name since it's going to be used a lot, at least I'd call it line

i here is for "interactive". If in general approach is fine I will change commit anyway.

if (auto lineInteractive = getInteractive(false)) {
// ...
}

This does not work good in case selectors without break.
And not very important here.

silverweed · 2026-04-10T11:26:37Z

graf2d/graf/inc/TLine.h

   Double_t      fY1{0};           ///< Y of 1st point
   Double_t      fX2{0};           ///< X of 2nd point
   Double_t      fY2{0};           ///< Y of 2nd point
+   std::unique_ptr<ROOT::Internal::TLineInteractive> fInteractive; ///<! temporary object for interactivity handling


Is there a strong reason to make it a unique_ptr rather than embedding the fields directly? (or as a struct value)

Sure, you'd have to define the internal class in the header, and it would take up more space in the TLine instance, but is that really a problem?

I mention - I see it as template for other classes.
There one can have big arrays used only during interactivity.
Also for TLine it is an issue - if you have 1000 of them at once.
Use of std::unique_ptr<> introduces only 8 extra bytes in size - which remain constant disregard of complexity of TLineInteractive class

linev · 2026-04-10T15:19:34Z

I closing this PR - probably idea with thread_local is more elegant.

linev added 2 commits April 10, 2026 09:47

[graphics] improve TArrow drawing

4892abb

Use PaintArrowNDC in the TArrow::Paint Use attributes ModifyOn methods Shorten options handling code

linev requested review from bellenot, dpiparo and silverweed April 10, 2026 11:04

linev self-assigned this Apr 10, 2026

linev added affects:master in:Graphics labels Apr 10, 2026

linev requested a review from couet as a code owner April 10, 2026 11:05

linev marked this pull request as draft April 10, 2026 11:05

silverweed reviewed Apr 10, 2026

View reviewed changes

linev closed this Apr 10, 2026

linev deleted the improve_graphics branch April 10, 2026 15:19

Conversation

linev commented Apr 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linev commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants