Implement @layer based on local variables?

[guo-yong-zhi]

Currently, the @layer is a shortcut for gsave() ... grestore(), where the gsave/grestore use the global stack _SAVED_COLORS_STACK to track the current state. It is inelegant and errorprone. Additionally, it makes multithreaded programming difficult. Says #238 @oheil :

I tried but failed to use https://github.com/tkf/ContextVariablesX.jl . I think it's because Luxor not only needs a context var x but a set of arrays for multi drawing.

Could we use local variables instead of global ones? Here is a simple and clear implementation, which doesn't need_SAVED_COLORS_STACK and _saved_colors anymore:

function layer(func)
    Cairo.save(get_current_cr())
    r, g, b, a = (get_current_redvalue(),
                  get_current_greenvalue(),
                  get_current_bluevalue(),
                  get_current_alpha()
                 )
    res = func()
    Cairo.restore(get_current_cr())
    set_current_redvalue(r)
    set_current_greenvalue(g)
    set_current_bluevalue(b)
    set_current_alpha(a)
    res
end

macro layer(a)
    layer(()->$(esc(a)))
end

Unfortunately, it would be a breaking change for the user of gsave and grestore (but not for the user of @layer). If you think this is a good idea, I can polish it and make a PR at the right time.

[oheil]

We are not using global variables. Those state vars needed for thread safety, like _SAVED_COLORS_STACK are hard local scoped using a let block, see https://docs.julialang.org/en/v1.11/manual/variables-and-scoping/#Let-Blocks
Access to the variables is through functions in global scope, which is fine. This is one of the recommended design patterns exactly to avoid globals.

In general, for breaking changes I am not the one to decide.

There is no need to remove gsave() and grestore(), this would avoid the breaking change. function layer is essentially what we had, before the changes I introduced for thread safety, so my guess is, this is not what we want.

I would prefer to talk about real issues. Solving issues on base of general statements like "It is inelegant and errorprone" are typically not solving anything but still consume time. A general redesign of the Luxor package is definitely not on my scope. If you could provide a suggestion to get rid of the use of threadid() I would be very happy.

[guo-yong-zhi]

Thanks for your pointing out the concept confusion. Yes, _SAVED_COLORS_STACK itself is not in global scope, while it serves publicly through the public api _saved_colors. What's essential is that globally shared state variables would be obstacles for parallel programming and also harm to readability. It's ok, but not ideal. So it should be cautious to use them.
As for getting rid of threadid() and further make a package fully parallelizable, I think there are two approaches:

1. Find a way to identify different task or manage the task local storage.
   Libraries like TaskLocalValues.jl and MultiThreadedCaches.jl may help, however,

We think this is not an ideal way to proceed, but it may make transitioning to safer code easier and require fewer rewrites. -- PSA

2. Do not use global/shared state variables.
   I think this is the right way to go, the ultimate solution for parallel programming.

This issue and that reply are something conceived in my mind, which is on a very early stage. I share it here for discussion.
I know that a "perfect" but "subversive" solution may be never adopted in an already stable package. Perhaps after many years, when the package is weighed down with too many "real issues", it is time to consider this. Or, perhaps I would refactor it (as an experimental fork) just in the next week. Yes, I am a big fan of parallel computing.

[oheil]

What's essential is that globally shared state variables would be obstacles for parallel programming and also harm to readability.

Except that in this case the global state is essentially needed so that Luxor.jl can be used from a parallel running consumer code. Luxor.jl itself doesn't use parallelism of any kind.

1. Find a way to identify different task or manage the task local storage.
   Libraries like TaskLocalValues.jl and MultiThreadedCaches.jl may help, however,

Only works if we/Luxor.jl itself uses internally threads. This is not the case. The problem is, that Luxor.jl is called from parallel running threads and has no control about this implementation from where it is called.

2. Do not use global/shared state variables.
   I think this is the right way to go, the ultimate solution for parallel programming.

Yes, but moves the problem to a general redesign and frankly, I doubt that a clean thread safe wrapper around Cairo can be written. It would be interesting to experiment with Cairo's save and restore feature, my guess is, enough threads would bring us into trouble. Well, for me far too academic to actually start such a thing.

This issue and that reply are something conceived in my mind, which is on a very early stage. I share it here for discussion.

Of course, I have seen this already, and @cormullion response is clear. Despite that, perhaps I will try it out and see if this really resolves the problem with @spawn. If this is the case, I would do the internal work.

I know that a "perfect" but "subversive" solution may be never adopted in an already stable package. Perhaps after many years, when the package is weighed down with too many "real issues", it is time to consider this. Or, perhaps I would refactor it (as an experimental fork) just in the next week. Yes, I am a big fan of parallel computing.

If there would be real issues, but there aren't. We are still on the abstract general concerns level. Luxor.jl is doing well since years and I don't believe it will run into many trouble in the coming years. Again, I am against solutions without real problems. There is not a thing like "perfect" or "subversive" solution, there are only solutions of real problems. We had yours, we solved it. This is a solution. Your refactored code isn't a solution, it's an alternative implementation, which is something worthwhile on it's own, but I will not have time for any project like this, which is the reason I talk so much about this ;-)

[guo-yong-zhi]

Thanks for your patience and your time.

see if this really resolves the problem with @spawn

I already tried this.
https://github.com/guo-yong-zhi/Luxor.jl/blob/master/test_parallel.jl is the test code.
Here is the reproduction steps:

• git clone my fork of Luxor.jl and cd into it
• julia -t 4
• ]dev .
• evalfile("./test_parallel.jl")

Here is the result:

test1
All output files of t1:
["2.png", "6.png", "8.png", "9.png"]

test2
All output files of t2:
["1.png", "10.png", "2.png", "3.png", "4.png", "5.png", "6.png", "7.png", "8.png", "9.png"]

I first refactored the @layer as mentioned in this issue, so the Context (in last reply) can be degraded to a plain Drawing. And I pass it to drawing functions by a keyword argument drawing= instead of a positional argument as I did in the last reply.
After the refactoring, it works within the @spawn block. Moreover, I think an explicit keyword drawing= is much more clear than calling _get_current_xxxx() everywhere in source code.

The code is just a demonstration, and I can see some flaws in the result pictures. But I think it's enough to demonstrate the feasibility of my proposal.

[cormullion]

In general, for breaking changes I am not the one to decide.

I think most would agree that a breaking change is worth imposing if there are substantial gains to be had. Which isn't really the case here. Now, if someone rewrote the text/font functions to improve font selection and text handling, and upgraded LaTeX/Typst functionality, that would certainly be worth the pain of going to a breaking release, judging from the issues raised.

As I commented here:

Perhaps there is room for a new ‘library-level’ package interface to Cairo.jl, suitable for use as graphics engine for higher-level packages.

Have you considered forking Luxor and modernizing it with your preferred approaches to drawing creation and use of contexts? MultiLuxor or PLuxor perhaps aren't the best names, but it might be that a drawing package redesigned to work better in modern applications (such as ML) would be welcomed by some.

Luxor.jl is 10 years old next month. Maybe this is a good time for a redesign.

[hustf]

Luxor is 10 years old, Cairo is 22 years old, and Pango is 25. They carry some luggage and inelegant, work-hardened solutions.

A lot of the good Luxor stuff is missing in Images.jl-land, bar typesetting. I don't think typesetting should ideally be integrated too tightly into graphics production, given the immense complexities of typesetting.

[guo-yong-zhi]

I think most would agree that a breaking change is worth imposing if there are substantial gains to be had. Which isn't really the case here.

Just to clarify, making Luxor @spawnable does not require a breaking change. The breaking change is casued by the local version of @layer that proposed in this issue. There are other ways to deal with the _SAVED_COLORS_STACK, I will illustrate an un-breaking alternative here.
I think this issue should be closed, since no one likes a breaking change. I opened it because I thought it's a minor breaking change. People use @layer a lot, but they use gsave and grestore very little.
If someone wants to continue talking about a @spawnable Luxor, maybe that issue is a more suitable place.

[cormullion]

Thanks for participating @guo-yong-zhi !