Re: [master] 9df287b - Cleaner access to Fl_Gl_Window_Driver objects.

[Albrecht Schlosser]

On 4/29/21 10:47 AM ManoloFLTK wrote:

commit 9df287b415cd4d67a7a371c4cc89a11ebb8340f6
Author:     ManoloFLTK <41016272+...@users.noreply.github.com>
AuthorDate: Thu Apr 29 10:40:02 2021 +0200
Commit:     ManoloFLTK <41016272+...@users.noreply.github.com>
CommitDate: Thu Apr 29 10:40:18 2021 +0200

    Cleaner access to Fl_Gl_Window_Driver objects.
...

diff --git src/Fl_Gl_Window_Driver.H src/Fl_Gl_Window_Driver.H
index bebde10..ae2c66f 100644
--- src/Fl_Gl_Window_Driver.H
+++ src/Fl_Gl_Window_Driver.H
@@ -100,6 +100,7 @@ public:
   virtual int genlistsize() { return 0; } // support for gl_draw()
   virtual Fl_Font_Descriptor** fontnum_to_fontdescriptor(int fnum);
   virtual Fl_RGB_Image* capture_gl_rectangle(int x, int y, int w, int h);
+  static inline Fl_Gl_Window_Driver* driver(const Fl_Gl_Window *win) {return win->pGlWindowDriver;}
 };
... 
 #endif /* Fl_Gl_Window_Driver_H */
diff --git src/drivers/OpenGL/Fl_OpenGL_Display_Device.cxx src/drivers/OpenGL/Fl_OpenGL_Display_Device.cxx
index 121c0db..1d0b25c 100644
--- src/drivers/OpenGL/Fl_OpenGL_Display_Device.cxx
+++ src/drivers/OpenGL/Fl_OpenGL_Display_Device.cxx
@@ -40,7 +40,7 @@ Fl_OpenGL_Display_Device::Fl_OpenGL_Display_Device(Fl_OpenGL_Graphics_Driver *gr
 
 Fl_RGB_Image* Fl_OpenGL_Display_Device::capture_gl_rectangle(Fl_Gl_Window* glw, int x, int y, int w, int h)
 {
-  return glw->gl_driver()->capture_gl_rectangle(x, y, w, h);
+  return Fl_Gl_Window_Driver::driver(glw)->capture_gl_rectangle(x, y, w, h);
 }
 
 /* Captures a rectangle of a Fl_Gl_Window and returns it as a RGB image.
diff --git src/drivers/X11/Fl_X11_Gl_Window_Driver.cxx src/drivers/X11/Fl_X11_Gl_Window_Driver.cxx
index a52d4c7..ac7041f 100644
--- src/drivers/X11/Fl_X11_Gl_Window_Driver.cxx
+++ src/drivers/X11/Fl_X11_Gl_Window_Driver.cxx
@@ -430,7 +430,7 @@ void _Fl_Gl_Overlay::draw() {
   uchar save_valid = w->valid();
   w->valid(valid());
   Fl_Xlib_Graphics_Driver::fl_overlay = 1;
-  w->gl_driver()->draw_overlay();
+  Fl_Gl_Window_Driver::driver(w)->draw_overlay();
   Fl_Xlib_Graphics_Driver::fl_overlay = 0;
   valid(w->valid());
   w->valid(save_valid);

Manolo, many thanks for all your continuous work on the driver stuff.

Here I'm wondering if it's "a good idea" to add that static method to return the driver.

My second question is if we really need  'capture_gl_rectangle()' rather than having a 'virtual capture_rectangle()' method that automatically does "the right thing"?

What about using the Fl_Window object to do the stuff we need to do, maybe with functions like these:

(1) virtual Fl_RGB_Image * Fl_Window::capture_rectangle(x, y, w, h);

This method would have (virtual) reimplementations in all subclasses and work with Fl_Gl_Window as well.

(2) virtual Fl_Window::draw_overlay();

This method would do nothing for normal windows but would have (virtual) reimplementations in all subclasses that need it. Maybe it could only be defined for classes that require it (not in the base Fl_Window class).


With such methods the driver stuff would be even more hidden and the virtual methods could do what we need.

Sorry if I'm not seeing the whole picture (I didn't investigate the relevant code parts), it's more a gut feeling in the sense: this must be easier to achieve...

Looking forward to your thoughts and explanations...

[Manolo]

On Thursday, April 29, 2021 at 12:36:59 PM UTC+2 Albrecht Schlosser wrote:

Here I'm wondering if it's "a good idea" to add that static method to return the driver.

With that, we have similar ways to access to Fl_Window_Driver and Fl_Gl_Window_Driver objects

of the form

static Fl_XX_Driver *Fl_XX_Driver::driver(Fl_XX *object)

These methods are not part of the public API, because they are in the drivers, and the driver member

of each class remains private. It would be possible to make the driver member public, but they would become

visible to the public API.

My second question is if we really need  'capture_gl_rectangle()' rather than having a 'virtual capture_rectangle()' method that automatically does "the right thing"?

What about using the Fl_Window object to do the stuff we need to do, maybe with functions like these:

(1) virtual Fl_RGB_Image * Fl_Window::capture_rectangle(x, y, w, h);

This method would have (virtual) reimplementations in all subclasses and work with Fl_Gl_Window as well.

(2) virtual Fl_Window::draw_overlay();

This method would do nothing for normal windows but would have (virtual) reimplementations in all subclasses that need it. Maybe it could only be defined for classes that require it (not in the base Fl_Window class).

The current situation is that we have hierarchies of classes and derived classes with virtual methods whose code is

platform-independent. Virtual methods allow for example to implement the resize operation adequately for widgets, groups, and windows.

Orthogonal to that, we have hierarchies of driver classes with derived classes for each supported platform. These methods

contain all the platform-specific code present in FLTK.

That situation is visible with the widget tree to which 2 driver trees are connected, those rooted at Fl_Window_Driver and at Fl_Gl_Window_driver.

It's visible also in the tree rooted at Fl_Surface_Device, connected to trees rooted at Fl_Image_Surface_Driver and at Fl_Copy_Surface_Driver.

With Fl_Gl_Window objects, there's an additional strong constraint that all GL-related code must be in libfltk_gl whereas libfltk

cannot use any member function of class Fl_Gl_Window. I'm not sure a single Fl_Window class with derived classes both for special

windows and for all platforms can accomodate that constraint.

My view is that this division in two orthogonal hierarchies is easily understandable and allows to hide completely all platform-specific

aspects from the FLTK public API. For sure that is the solution to which we, Matthias and I,  arrived when we decided to convert FLTK

to the driver model. Others may exist, but this one does work, and has proven able to accomodate new platforms (e.g., Android)

without any change to the platform-independent code base.

[Albrecht Schlosser]

On 4/29/21 9:16 PM Manolo wrote:
>
> On Thursday, April 29, 2021 at 12:36:59 PM UTC+2 Albrecht Schlosser wrote:
>
> Here I'm wondering if it's "a good idea" to add that static method
> to return the driver.
>
> With that, we have similar ways to access to Fl_Window_Driver and
> Fl_Gl_Window_Driver objects of the form
>
> static Fl_XX_Driver *Fl_XX_Driver::driver(Fl_XX *object)

> These methods are not part of the public API, because they are in the
> drivers, and the driver member
> of each class remains private. It would be possible to make the driver
> member public, but they would become
> visible to the public API.

Okay, understood and accepted. The static method avoids making the
driver() method public and that's good.

So please ignore that part for further discussion.

> My second question is if we really need  '/capture_gl_rectangle()/'
> rather than having a '/virtual capture_rectangle()/' method that

> automatically does "the right thing"?
>
> What about using the Fl_Window object to do the stuff we need to do,
> maybe with functions like these:
>
> (1) virtual Fl_RGB_Image * Fl_Window::capture_rectangle(x, y, w, h);
>
> This method would have (virtual) reimplementations in all subclasses and work with Fl_Gl_Window as well.
>
> (2) virtual Fl_Window::draw_overlay();
>
> This method would do nothing for normal windows but would have
> (virtual) reimplementations in all subclasses that need it. Maybe it
> could only be defined for classes that require it (not in the base
> Fl_Window class).
>
> The current situation is that we have hierarchies of classes and derived
> classes with virtual methods whose code is

> platform-independent. Virtual methods ...

Sure.

> Orthogonal to that, we have hierarchies of driver classes with derived
> classes for each supported platform. These methods
> contain all the platform-specific code present in FLTK.

Okay, but this has nothing to do with my second question to introduce
the methods (1) and (2) above.

Please look back at my suggestion (1). That point has nothing to do with
internal driver classes.

You could also take it as a general RFE. We have fl_read_image() but
this returns only a data array. The proposed function would return an
Fl_RGB_Image.

> With Fl_Gl_Window objects, there's an additional strong constraint that
> all GL-related code must be in libfltk_gl whereas libfltk
> cannot use any member function of class Fl_Gl_Window. I'm not sure a
> single Fl_Window class with derived classes both for special
> windows and for all platforms can accomodate that constraint.

Fl_Gl_Window /is/ derived from Fl_Window anyway. So you can define

virtual Fl_RGB_Image * Fl_Window::capture_rectangle(x, y, w, h);

and redefine/reimplement it in class Fl_Gl_Window. This can even be done
if the implementation of Fl_Window::capture_rectangle() is in lib fltk
and Fl_Gl_Window::capture_rectangle() is in libfltk_gl.

This method /could/ also be public, there's no reason not to let users
use this.

So let me rephrase my suggestion. What about adding

virtual Fl_RGB_Image * Fl_Window::capture_rectangle(x, y, w, h);

to the public API in addition to fl_read_image() ?

> My view is that this division in two orthogonal hierarchies ...

Again, the proposal above has nothing to do with the internal
implementation of drivers. It's just an implementation of virtual
functions in derived classes Fl_Window and Fl_Gl_Window, resp.. The
final implementation in either class would presumably use the
Fl_Window_Driver and Fl_Gl_Window_Driver internally, but that's the case
anyway. (As you say, this is orthogonal to the widget hierarchy.)


PS: I'm not sure whether proposal (2) is as useful as (1), so let's not
talk about (2) for now. I need to look into more details first.

[Manolo]

@Albrecht:

OK. I had misunderstood your initial proposal. It's not about reconsidering all

trees of classes. It's just about adding to the public API a function to capture

a rectangle in a window, whatever the nature of that window, and to return an

Fl_RGB_Image object. Is that correct ?

That function already exists, it's fl_capture_window_part()

     see https://fltk.gitlab.io/fltk/group__fl__drawings.html#ga74c16eb73f1a1be989a16da9086622f0

The returned Fl_RGB_Image is scaled (with Fl_Image::scale()) so that its drawing size

is the size of the rectangle asked for, and its data size is the number of pixels present

in the captured rectangle which varies with the display's scaling factor.

Thus, it's not a public member of class Fl_Window as you proposed, but it's functionally identical.
We have the possibility to change that since 1.4 is not out yet. Let me know what you believe is best.

However,  I have put this feature in the form of a function rather than a member of class

Fl_Window, I believe, because it is the correct extension of the classic fl_read_image() function

for the situation arising with FLTK 1.4 and HighDPI displays :  behind rectangle

X,Y,W,H in FLTK units there are many more than WxH pixels, and the returned

uchar array of function fl_read_image() is not able to convey that information.

The doc of fl_read_image() proposes a link to the new fl_capture_window_part() :

   https://fltk.gitlab.io/fltk/group__fl__drawings.html#ga0cdc05d3f7689e1c6b8a26fd0bd97233

Thinking about that, we could link to a new Fl_Window::capture_rectangle() member function

just as well.

[Albrecht Schlosser]

On 4/30/21 10:02 AM Manolo wrote:
> @Albrecht:
> OK. I had misunderstood your initial proposal. It's not about
> reconsidering all trees of classes.

Certainly not!

> It's just about adding to the public API a function to capture
> a rectangle in a window, whatever the nature of that window, and to
> return an Fl_RGB_Image object. Is that correct ?

Yes, more or less, we can reduce it to that. Sorry for the confusion, my
"request" was really unclear. It was a quick shot when I saw the commit.

> That function already exists, it's fl_capture_window_part()
>      see
> https://fltk.gitlab.io/fltk/group__fl__drawings.html#ga74c16eb73f1a1be989a16da9086622f0
> The returned Fl_RGB_Image is scaled (with Fl_Image::scale()) so that its
> drawing size is the size of the rectangle asked for, and its data size
> is the number of pixels present in the captured rectangle which
> varies with the display's scaling factor.
>
> Thus, it's not a public member of class Fl_Window as you proposed, but
> it's functionally identical.
> We have the possibility to change that since 1.4 is not out yet. Let me
> know what you believe is best.

I strongly believe that this should be a public method of Fl_Window and
derived classes. Based on what you and the docs say (thanks for that!) a
very simple functional solution would be:

(pbulic) Fl_RGB_Image * Fl_Window::capture(x, y, w, h) const {
return fl_capture_window_part(this, x, w, w, h);
}

However, I'd rather like it technically refactored as a normal Fl_Window
method.

> However, I have put this feature in the form of a function rather than
> a member of class Fl_Window, I believe, because it is the correct
> extension of the classic fl_read_image() function for the situation
> arising with FLTK 1.4 and HighDPI displays :
> behind rectangle
> X,Y,W,H in FLTK units there are many more than WxH pixels, and the returned
> uchar array of function fl_read_image() is not able to convey that
> information.

Yes, I agree this is a good reason to implement the new method.

> The doc of fl_read_image() proposes a link to the new
> fl_capture_window_part() :
>
> https://fltk.gitlab.io/fltk/group__fl__drawings.html#ga0cdc05d3f7689e1c6b8a26fd0bd97233

Yes, I see...

> Thinking about that, we could link to a new
> Fl_Window::capture_rectangle() member function just as well.

A missing piece of Fl_Window::capture_rectangle(), compared to
fl_read_image() would be that fl_read_image() can create an alpha
channel in case the user wants one. IIRC this would be a constant for
all pixels.

In the docs of fl_capture_window_part() you write: "The image depth may
differ between platforms". What exactly does this mean? Are there
platforms (macOS?) where you can "capture transparency" from an existing
window? Or what else does it mean?

If this (alpha channel on certain platforms) does not preclude it, we
could also add an optional 5th parameter 'alpha' to return an image with
a preset alpha channel. Maybe. I don't know the details, this should be
discussed further.

Regarding the implementation as Fl_Window method or a function: I think
normal methods are more natural and easier to find. I've never been a
friend of such functions like fl_read_image(), they're hard to find in
the docs and not intuitive.

Regarding the naming convention: I believe that '_window' and '_part'
are obsolete if we define an Fl_Window method, hence my short proposal
(now with optional alpha):

Fl_Window::capture( x, y, w, h [, alpha] );

If we add this we can /deprecate/ fl_read_image() and point the user at
the new method.

What do you think?

[Bill Spitzak]

I would prefer if the call to get an image was in the same namespace as calls to send an image, thus not methods of Fl_Window, unless all drawing was made into methods of Fl_Window.

It would make sense to update the api so that it can be implemented by returning pointers to memory already allocated by the drawing library, however.

--
You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/9b62055d-45d7-cb91-05a1-50f06d1f1bd8%40online.de.

[Manolo]

On Friday, April 30, 2021 at 9:35:36 PM UTC+2 Albrecht Schlosser wrote:

A missing piece of Fl_Window::capture_rectangle(), compared to
fl_read_image() would be that fl_read_image() can create an alpha
channel in case the user wants one. IIRC this would be a constant for
all pixels.

In the docs of fl_capture_window_part() you write: "The image depth may
differ between platforms". What exactly does this mean? Are there
platforms (macOS?) where you can "capture transparency" from an existing
window? Or what else does it mean?

GL windows return depth-3 images on all platforms.
Other windows return depth-4 on macOS and depth-3 on X11 and Windows.

Because the capture can mix GL and non-GL windows, depth conversions

are performed by the function when necessary.

I kept the depth-4 on macOS because this preserves the rounded angles of the window

when the image is drawn: off-window pixels are fully transparent. There's no benefit

for most pixels, because FLTK doesn't (yet) draw with transparency.

 

If this (alpha channel on certain platforms) does not preclude it, we
could also add an optional 5th parameter 'alpha' to return an image with
a preset alpha channel. Maybe. I don't know the details, this should be
discussed further.

What is the use of a constant transparency value across all pixels of an image?

If there's one, it's certainly possible to apply it when drawing that image.

[Albrecht Schlosser]

On 5/1/21 9:30 AM Manolo wrote:
>
> On Friday, April 30, 2021 at 9:35:36 PM UTC+2 Albrecht Schlosser wrote:
>
> A missing piece of Fl_Window::capture_rectangle(), compared to
> fl_read_image() would be that fl_read_image() can create an alpha
> channel in case the user wants one. IIRC this would be a constant for
> all pixels.
>
> In the docs of fl_capture_window_part() you write: "The image depth may
> differ between platforms". What exactly does this mean? Are there
> platforms (macOS?) where you can "capture transparency" from an
> existing window? Or what else does it mean?
>
> GL windows return depth-3 images on all platforms.
> Other windows return depth-4 on macOS and depth-3 on X11 and Windows.
> Because the capture can mix GL and non-GL windows, depth conversions
> are performed by the function when necessary.

Okay, thanks for the explanation.

> I kept the depth-4 on macOS because this preserves the rounded angles of
> the window when the image is drawn: off-window pixels are fully transparent.
>

> If this (alpha channel on certain platforms) does not preclude it, we
> could also add an optional 5th parameter 'alpha' to return an image
> with
> a preset alpha channel. Maybe. I don't know the details, this should be
> discussed further.
>
> What is the use of a constant transparency value across all pixels of an
> image?

I don't know. I can imagine that you want to get an image for blending
or for later manipulating of the alpha channel. I think it's just
convenient to "say": please give me an image with a preset alpha channel.

> If there's one, it's certainly possible to apply it when drawing that image.

My point /was/ backwards compatibility. If we wanted to deprecate
fl_read_image() and tell users to use the new Fl_Window::capture()
method instead, then this method needed to provide the same "features".

OTOH, thinking... If it's just different, then so may it be. We can't
remove the old method anyway and the output format is different ... Yes,
please forget compatibility, let's just add this new method as an
alternative to fl_read_image() with a different output format
(Fl_RGB_Image*).

[Albrecht Schlosser]

On 4/30/21 10:52 PM Bill Spitzak wrote:
> I would prefer if the call to get an image was in the same namespace as
> calls to send an image, thus not methods of Fl_Window, unless all
> drawing was made into methods of Fl_Window.

Bill, thanks for your comment. I understand the words, but I don't
understand what you exactly mean and particularly *why* you think that
would be better.

WRT drawing methods: as an example we have Fl_[derived_]Image::draw()
and we have fl_draw_image(), i.e. we have in this case two entirely
different draw() methods / functions. Both have their usages.

My proposal Fl_Window::capture() would have the additional advantage
that it could be virtual and be reimplemented differently in
Fl_Gl_Window w/o needing to query the window type in the function
fl_capture..(window, ...).

Looking forward to your reasoning...

> It would make sense to update the api so that it can be implemented by
> returning pointers to memory already allocated by the drawing library,
> however.

That's another point I really don't understand. What "api" should be
updated, and what do you mean with "returning pointers to memory already
allocated by the drawing library"?

Hmm, do you mean the api of fl_read_image()? This allows already to use
NULL for the pixel buffer so FLTK allocates the data buffer internally.
Is this what you mean?