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

Initial canvas implementation #1438

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Guldoman
Copy link
Member

@Guldoman Guldoman commented Mar 20, 2023

This is just the minimum required to support canvases.
A future PR will implement more methods to draw on top of canvases, and other utils.
Another future PR will optimize rencache to allow re-rendering only the part of canvases that changed (which will be useful for things like minimap).

Demo:

local function color_to_32(r, g, b, a)
  r, g, b, a = math.floor(r), math.floor(g), math.floor(b), math.floor(a or 255)
  return string.pack("I4", (r << 24) + (g << 16) + (b << 8) + a)
end

local pixels = {}
local n = 1
for j = 1, 200 do
  for i = 1, 200 do
    pixels[n] = color_to_32(i, j, math.sqrt(i * j), 100)
    n = n + 1
  end
end
pixels = table.concat(pixels)

local c = renderer.create_canvas(255, 255, {255, 0, 0, 255})
c:set_pixels(pixels, 0, 0, 200, 200)

local RootView = require "core.rootview"
local old_rootview_draw = RootView.draw
function RootView:draw(...)
  old_rootview_draw(self, ...)
  renderer.draw_canvas(c, self.mouse.x, self.mouse.y, self.mouse.x / self.size.x * 255, self.mouse.y / self.size.y * 255)
end

Closes #1182.

@Guldoman
Copy link
Member Author

It seems like we can't properly handle scaled renderer.draw_canvas (where the canvas needs to be scaled to fit the requested sizes).
This is because in rencache, when we re-render part of the canvas, the resulting clip suffers from rounding errors which impact how SDL does scaling.

In this video I'm drawing a tight grid at various scales. The mouse cursor is forcing the redraw of the rencache cell it's hovering.

Video.del.2023-03-21.01-36-00.webm

Notice how at 100% scale there are no rendering artifacts, but they appear at any other scale.

I think that this can be worked around by the future PR where I'll add drawing a canvas over another.
That would allow a canvas to be drawn scaled over another one, which can then be drawn via rencache without producing artifacts.

Another solution would be to keep a scaled surface cached somewhere, and use that one instead of the one in the canvas.
I'll investigate this solution.

If that doesn't work out, should we add a warning to avoid calling renderer.draw_canvas with scaled dimensions, or should we just disallow it?

@Guldoman
Copy link
Member Author

Another little edge case: what should we do if the canvas changes between the renderer.draw_canvas call and renderer.end_frame?
Should we draw the "old" version or the "new" one?

If we decide to draw the "old" version, we could probably solve the "scaled" surface problem at the same time.

@Guldoman
Copy link
Member Author

Another little edge case: what should we do if the canvas changes between the renderer.draw_canvas call and renderer.end_frame? Should we draw the "old" version or the "new" one?

With the latest commit this issue has been solved, and we now draw the surface with the "old" state.

@Guldoman
Copy link
Member Author

The latest commit fixes the scaling issue.

Now it's time for input validation and cleanup.

@Guldoman Guldoman force-pushed the PR_add_canvas branch 2 times, most recently from 823ce6e to 589a4a4 Compare March 24, 2023 14:38
When using `cmd->rensurface = *rs;`, the padding bytes were being copied 
too.
Issue found by valgrind.
This implements a COW cache to make `renderer.draw_canvas` operations 
result in their "current" state to be drawn on screen, and not their 
"altered" state.
This avoids artifacts caused by rounding errors in clipping.

When we need the scaled version of a canvas, we check if we have already
one cached in the current frame cache.
If not we check if it was cached in the previous frame, and if so, we
copy it to the current frame cache.

If the scaled canvas is not found, we create one and push it to the
current frame cache.

We keep the cache of the previous frame to avoid recreating the same
scaled canvas every frame.
@jgmdev
Copy link
Member

jgmdev commented Apr 11, 2023

I would suggest providing a ready to use color_to_32 function as on your example, preferably implemented on the C side maybe as part of renderer.

@Guldoman
Copy link
Member Author

I would suggest providing a ready to use color_to_32 function as on your example, preferably implemented on the C side maybe as part of renderer.

Do we need that?
Normally one should avoid doing image generation in Lua anyways, and should either load from a file that contains raw pixel data, or use a plugin to load the pixel data from an actual image.

A future PR will allow loading pixel data without going through Lua strings, but directly from a lightuserdata, which will be useful for native plugins.

Btw don't merge this PR yet, as I'm looking into optimizing draw operations by using a pixel format that's more in line with the window pixel format.
When the time comes, it might also be a good idea not to squash this, and keep the history of the commits.

@jgmdev
Copy link
Member

jgmdev commented Apr 14, 2023

Do we need that? Normally one should avoid doing image generation in Lua anyways, and should either load from a file that contains raw pixel data, or use a plugin to load the pixel data from an actual image.

The description mentions that a future PR will allow better re-rendering on only parts of the canvas that changed for stuff like minimap, which means that this "canvas" functionality could be also useful for plugins like https://github.com/ThaCuber/equationgrapher or https://github.com/vincens2005/lite-snake for more performant rendering or at least that is how I interpreted it...

Unless this allows the reuse of draw rect and text functions on top of the canvas, performing stuff like the mentioned minimap will require having to constantly deal with pixel data directly, so having a function that does the conversion for you already shouldn't be an optional thing. And as you may already know usually the canvas api provided on various platforms like web browsers, etc... is usually accompanied with various drawing functions for primitive shapes. People may expect more higher level facilities than what is actually provided by this "canvas" functionality which as you are pointing looks more like a generic image loader than what is known as an actual canvas

A future PR will allow loading pixel data without going through Lua strings, but directly from a lightuserdata, which will be useful for native plugins.

Maybe the design on the HTML5 canvas API could be used as a foundational idea, I'm not saying to implement all that functionality, but to have a solid idea of what its API should look like for something that is been called a "canvas" on Lite XL core.

Btw don't merge this PR yet, as I'm looking into optimizing draw operations by using a pixel format that's more in line with the window pixel format. When the time comes, it might also be a good idea not to squash this, and keep the history of the commits.

Don't worry, I don't believe such important feature as exposed on this PR is mature yet for something that will play an important role on future plugin development. The user facing API needs to be given more thought even if this PR is only going to implement the foundation for it.

@Guldoman
Copy link
Member Author

The description mentions that a future PR will allow better re-rendering on only parts of the canvas that changed for stuff like minimap, which means that this "canvas" functionality could be also useful for plugins like https://github.com/ThaCuber/equationgrapher or https://github.com/vincens2005/lite-snake for more performant rendering or at least that is how I interpreted it...

Unless this allows the reuse of draw rect and text functions on top of the canvas

As expressed multiple times on Discord, yes, that's one of the objectives of a future PR. canvas will support draw_rect, draw_text and set_clip from renderer an will get support for canvas on canvas blit.

And as you may already know usually the canvas api provided on various platforms like web browsers, etc... is usually accompanied with various drawing functions for primitive shapes.

We will not add support for those.

Don't worry, I don't believe such important feature as exposed on this PR is mature yet for something that will play an important role on future plugin development. The user facing API needs to be given more thought even if this PR is only going to implement the foundation for it.

The API of the basic canvas functions implemented in this PR can be already considered mostly stable. I'll likely only add a couple more parameters to deal with custom pixel formats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load and draw images
2 participants