move window creation and management to Lua

[Jan200101]

• remove scaling logic from font related rendering functions
  • before font loading require knowing the window scale, but we don't, and really can't, have a window that early.
• separate FreeType loading into a new function
  • FreeType needs to be loaded before windows are created so we can prepare fonts
• remove window_renderer global
• pass all window info via arguments
• keep track of all created RenWindows in a list
  • This is needed to deal with SDL events, since we act upon our internal RenWindow, not the raw SDL_Window.
  • keeping it in a list takes a few more CPU cycles, but uses less ram overall.
• rename RenWindow creation and cleanup functions to ren_create and ren_destroy
• reuse the ren_init and ren_free names for global allocations (draw_rect_surface, freetype)

Due to this change being quite invasive with no easy backwards compatibility, the mod verison would need to be bumped.

[Jan200101]

EDIT: will be done in a later PR

[Jan200101]

a discussion around this API change already happened on discord, so I'm marking this to get more feedback.

The current implementation requires that the first argument is the relevant window, but this change would be backwards incompatible.

Alternatives include:

• making window argument optional (checking arg count, putting it at the end, etc.)
  • this would make adding arguments more painful in the future
• turning the window into a global
  • Personally I am not the biggest fan of it because its likely going to introduce race conditions if done wrong.
  • The only way I could see this being done is by having a set and unset (push and pop?) function and erroring out if you are trying to set when its already unset
• leaving the existing draw functions as is and adding renwindow:draw_* functions which act directly on the relevant window
  • the renderer.draw_* methods could then be redirected to the main window methods
• implement a new draw_*_ex family of functions
  • same as the non ex version, except it takes a window instance (name is up to debate)
• accept that window is an argument of the draw function
  • this is way more explicit and ensures that intended behavior

[Guldoman]

Draw

Currently drawing only works (or at least, works without weird issues) when done between renderer.begin_frame() and renderer.end_frame():

lite-xl/data/core/init.lua

Lines 1391 to 1395 in 62d7ec8

	renderer.begin_frame()
	core.clip_rect_stack[1] = { 0, 0, width, height }
	renderer.set_clip_rect(table.unpack(core.clip_rect_stack[1]))
	core.root_view:draw()
	renderer.end_frame()

So no drawing in a core.add_thread coroutine, or in other random places.

If we don't want to allow mixing draws to a window and draws to another (which makes sense not to imho), we could pass the window to draw to as a parameter to renderer.begin_frame, so renderer.begin_frame(target_window).
This window will then be saved as a global variable on the C side, and will be accessed by the rendering functions.

Another possibility is to set it as a global variable on the Lua side, and retrieve it on the C side when needed by rendering functions, but this approach might slow things down a little bit (needs testing to see how much, if at all).

Both of these solutions would allow us to avoid breaking renderer API compatibility.

This is only valid for :draw(), but we need to also specify what happens on :update() and input functions.

Update + input

Some :update() functions might want to access window information, like its size and status, which are currently exposed by the system API.
So we need to decide what to do with those functions, if we want to move them inside window, or make them look up the currently targeted window from a global state.

If we move them to window, we need to find a way to expose that to its relative Views.

If we consider that each window will have its own RootView, we could let each RootView know about its window, and it would pass that on to its managed Views.

The issue with this approach is that RootView doesn't actually directly handle Views, but it sends instructions down the Node tree, so RootView doesn't directly know about the Views it contains.

So the possible solutions are:

1. Set the currently targeted window (when handling inputs and when handling :update()) as a global Lua variable that Views can just call window:get_size() on.
2. Set the currently targeted window as a global C variable which will be used when the "legacy" system and renderer functions are called.
3. Tell Nodes what window they belong to, and they'll pass this information on to their managed Views.

The issue with 1 and 2 is for example with dealing with core.add_thread coroutines, as they won't be able to access the window of the View they were spawned from (as the global will have already changed to another window), and would have to manually obtain that and save it locally.
With 3 this issue is not a problem, as the window would be saved inside the View itself (let's say View.window) which the coroutine would be able to access as an upvalue.

We also need to keep in mind how all of this will behave when a View is moved from a window to another.

Possible conclusions

We could have a single C-side global variable that will be used when the generic renderer.draw_* and system.get_window_* functions get called; let's say window.set_target_window sets that.

This would be called by RootView before forwarding input, update and draw calls.

Moreover RootView would also forward the window object to the Nodes tree, which would in turn set View.window on each View they manage, when added to them.

When a View is moved from a window to another, the generic APIs would just work, and the Node they're moved to would update the value of View.window.

This would allow us to keep the "legacy" API, and allow the usage of the window objects directly if/when needed.

Any thoughts?

[Jan200101]

Making a review comment to consider what Guldo said so replies can be focused.

I'm okay with simply setting a global active window when passed to renderer.begin_frame (with end_frame unsetting the global and doing sanity checks)

What about the other functions though, which aren't directly rendering related?
(such as system.set_window_title)?

[Jan200101]

Okay, all the rendering code no longer takes a window as a parameter and instead works with the active window for the current frame.

I'm probably going to move over the system window functions into renwindow and remove them completely since they are a lot more flexible and not suited for this.

[adamharrison]

ctrl+alt+r causes the window to disppear, and never reappear; just hanging the editor.

Ideally we'd keep the first window always open until lite-xl exits, so that we have a smooth restart, like we do now.

[Jan200101]

the restart issue has been fixed
a method to persist a single window has been added for the time being.

I'd say this PR is done
things like adapting sessions for multiple windows should be done in a follow-up

[takase1121]

Code-wise this is good aside from random inconsistent brace indents. As for the functionality I cannot comment much since I didn't actively participate in the discussion, but I will try to follow and see there are other points to consider.

[takase1121]

I read the comments here, and I have just a question.

1. how are the font functions going to scale correctly, for example, get_size() where it's impossible to determine which window it should be rendered on; in extension, if the windows are in different monitors with a multi-DPI setup, at what scale will it be calculating the size?)

Other than that, other aspects seem fairly okay to me; you'd be expected to pass various things into system, but also this is a chance to move those code into api/renwindow.c to make it more ergonomic. Since we're breaking modversion anyway, this should be considered (perhaps in a future PR).

[takase1121]

As for persisting the window, will it be worth it to preserve the window list somehow (I recall the windows are being stored in a list somewhere), so like windows that are not closed are preserved, and a Lua function can be called to re-associate them with a Lua userdata and get a list of them.

The GC will not "close" windows but disown them, and they're actually closed when the runtime exits (instead of restart).

[Guldoman]

1. how are the font functions going to scale correctly, for example, get_size() where it's impossible to determine which window it should be rendered on; in extension, if the windows are in different monitors with a multi-DPI setup, at what scale will it be calculating the size?)

Ideally the Lua side would supply the font size to the rendering functions, and it would not be a property of the font.

[Jan200101]

1. how are the font functions going to scale correctly, for example, get_size() where it's impossible to determine which window it should be rendered on; in extension, if the windows are in different monitors with a multi-DPI setup, at what scale will it be calculating the size?)

In my opinion ren_font_load should never have taken a window as an argument since fonts are one of the first things loaded.
Now it simply gets the requested size.

And its not like this is gonna cause major issues since scaling is busted as is (though may be worth documenting in #1750)

As for persisting the window, will it be worth it to preserve the window list somehow (I recall the windows are being stored in a list somewhere), so like windows that are not closed are preserved, and a Lua function can be called to re-associate them with a Lua userdata and get a list of them.

The GC will not "close" windows but disown them, and they're actually closed when the runtime exits (instead of restart).

I do agree that its worth preserving all windows.
But its not trivial to implement that without reworking a lot of things which I wish to do in a follow-up PR.

Currently the Windows has its lifetime managed in Lua and as soon as the garbage collector finds it dangling it will get destroyed.

I think windows should be managed entirely in Lua until ownership is given up (for example with a disown method, though we should probably also make an own method to reverse it)
but there may also be other use cases where disowning without persisting is needed so we probably need an internal _persist method that just does the disowning and then storing.
Next question is: how do you store it?
You could just store a list of windows, which would be the simplest solution and then have a lua method _restore which would return a list of all the persisted windows.

the above solution is the best I can think of right now, but I really really really do not want to pack this PR with any more stuff and think this can be best put into a separate, smaller PR to make it easier to review, test, merge as well as reason about at a later date.

[Jan200101]

another topic regarding windows storage:
what if a third party plugin wants to store a window?
we can't just bunch it up with the rest since that would make it impossible for core to find its windows.
perhaps we need a way to ID/Tag windows?

[takase1121]

another topic regarding windows storage: what if a third party plugin wants to store a window? we can't just bunch it up with the rest since that would make it impossible for core to find its windows. perhaps we need a way to ID/Tag windows?

Not sure if this would work, but you could probably store it as a lightuserdata (basically a C pointer).

[Jan200101]

Outside of bikeshedding I consider this PR done as is and any additions should be put into a follow-up PR

[takase1121]

I'm ok to merge this PR, how about @Guldoman and @adamharrison?

[adamharrison]

There is a noticeable period where the window is just blank before first draw that I don't notice with the current build; this could be either an actual regression in terms of performance, or just a seeming one where we're showing the window earlier.

It's not necessarily a blocker for me, but if we can resolve it to have roughly the same performance (either real or imagined) as before, that'd be good. I'll do some investigation in a bit to see if it's simply a hiding window thing.

[Jan200101]

I've tried recreating the blank window thing and can't.
The flow of the code is the same, the window gets created and then used down the line.

[adamharrison]

Found the issue. It's because on master we pass SDL_WINDOW_HIDDDEN.

You call SDL_ShowWindow on the window, but it shows by default if you have this flag, so the check for the initial frame is superfluous and leads to this behavior (at least on my setup)

So what I'd suggest we do is change the creation to:

  SDL_Window *window = SDL_CreateWindow(
    title, x, y, width, height,
    SDL_WINDOW_RESIZABLE | SDL_WINDOW_ALLOW_HIGHDPI | SDL_WINDOW_HIDDEN
  );

and change the update_rects call to:

void ren_update_rects(RenWindow *window_renderer, RenRect *rects, int count) {
  static bool initial_frame = true;
  renwin_update_rects(window_renderer, rects, count);
  if (initial_frame) {
    renwin_show_window(window_renderer);
    initial_frame = false;
  }
}

This fixes the issue on my end. Technically, actually, we should probably store initial_frame inside RenWindow, because the static will make any new window past the first think it's already had an initial frame.

[adamharrison]

Looks good! Merging.

[Jan200101]

for reference, since this was discussed on Discord:
I vaguely recall having issues with SDL_WINDOW_HIDDEN but since this PR is part of a bigger change it can be readded, might have been new windows not being displayed due to the inital frame logic.
Adding initial_frame to RenWindow should probably be done, but since with this PR everything is still based on a single window it won't affect anyone but potential plugin developers.