Skip to content

perf(input): cut per-character render cost (D2) #41

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.

Sign up for GitHub

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

Open
Vadim1987 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

perf(input): cut per-character render cost (D2) #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/util/view.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ end
--- @param selected boolean
local write_token = function(dy, dx, token,
color, bgcolor, selected)
gfx.push('all')
local r, g, b, a = gfx.getColor()

@aldum aldum Jul 3, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is actually faster than moving the stack pointer?
And more importantly, that no other state needs to be managed? It's possible, but I'd like to be sure.

@Vadim1987 Vadim1987 Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went read the 11.x source to check myself again. From what I see, push('all') is more than a stack pointer move: Graphics::push with STACK_ALL does states.push_back(states.back()) (Graphics.cpp:1622), and DisplayState carries quite a lot — both colors, blend, line settings, scissor, stencil, depth, cull, winding, a StrongRef and StrongRef, and the render target list (a heap vector). pop() then goes through restoreStateChecked (:409), which re-applies fields one by one — some setters run unconditionally (setFont/setShader among them), and the popped copy gets destroyed after. The way I read it, the color-only variant is a subset of that work — restoreStateChecked itself calls setColor when the color differs, so we're keeping that part and dropping the rest. That's mostly why I felt safe calling it cheaper, though I haven't benchmarked it — can do that if you'd like actual numbers. On other state: as far as I can tell, write_token only ever calls setColor and print (print reads the font and transform but doesn't change anything), so color seemed to be the only thing to manage. I did add a small spec pinning "the active color is left untouched" — the thought was, if the function ever needs to touch more state, widening the save back becomes a visible step instead of something to remember. But you know this code's history better — if there's state I'm not seeing, happy to go back to push('all').

if selected then
gfx.setColor(color)
local back = string.rep('█', string.ulen(token))
Expand All @@ -46,7 +46,7 @@ local write_token = function(dy, dx, token,
gfx.setColor(color)
end
gfx.print(token, dx, dy)
gfx.pop()
gfx.setColor(r, g, b, a)
end

--- Hide elements for debugging
Expand Down
7 changes: 0 additions & 7 deletions src/view/input/userInputView.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ function UserInputView:render_input(input, status, time)
local apparentHeight = inHeight

local wrap_forward = vc.wrap_forward
local wrap_reverse = vc.wrap_reverse


local vpH = gfx.getHeight()
Expand Down Expand Up @@ -166,7 +165,6 @@ function UserInputView:render_input(input, status, time)
local char = string.usub(s, c, c)
local color = colors.fg

local hl_li = wrap_reverse[ln]
local tlc = vc:translate_from_visible(Cursor(l, c))

if tlc then
Expand Down Expand Up @@ -204,11 +202,6 @@ function UserInputView:render_input(input, status, time)
end
end
end)()
local of = calc_overflow(w, text, cursorInfo.cursor)
--- push any further lines down to display phantom line
if ofpos and hl_li > cl then

@aldum aldum Jul 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this functionality being removed without any replacement? I understand it's not the most elegant solution, and would welcome a principled concept on how to do it well, but it has a purpose, don't just drop it.

@Vadim1987 Vadim1987 Jul 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — I should have put the history in the PR text instead of just calling it dead, that's on me. Here's what I found when I traced it. Before 237e625 ("fix(uiv): proper drawing of wrapped lines + DRY") the adjustment had a reader — it fed the y coordinate directly:
local of = overflow
--- push any further lines down to display phantom line
if ofpos and hl_li > cl then
of = of - 1
end
local dy = y - (-ln - diffset + 1 + of) * fh
That commit switched dy to visible space (dy = (l) * fh), and from that point of is computed, conditionally decremented, and not read by anything — it also started being recomputed through calc_overflow for every character instead of reusing overflow from the top of render_input. The purpose itself — making room for the phantom line — didn't go away; as far as I can tell it moved upstream in that same change: the single calc_overflow call at the head of render_input feeds overflow into inHeight/start_h, and of_h goes into the cursor row (vcl = acl - vc.offset + of_h). So my reading was that the loop block is the leftover of the old dy formula rather than functionality waiting for a replacement — the replacement is the visible-space math that's already there. On the principled concept: I'd argue it's the one that commit introduced — coordinates come from visible space, and phantom-line accounting happens once per frame in the visible-range computation, not per character in the draw loop. If phantom rendering has gaps today (I haven't found one, but I mostly looked at this path), they'd be in calc_overflow / of_h, and I'd rather fix them there — happy to dig into that separately. If it's easier to reason about, I can split this PR in two — the write_token change and the removal — or keep the block and hoist it out of the character loop for now. Whatever works for you.

of = of - 1
end
local dy = (l) * fh
local dx = (c - 1) * fw
ViewUtils.write_token(dy, dx,
Expand Down
45 changes: 45 additions & 0 deletions tests/util/view_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
local mock = require("tests.mock")
mock.mock_love({})
require("util.view")

describe('view utils #view', function()
local prev_gfx
local color

setup(function()
prev_gfx = _G.gfx
color = { 1, 1, 1, 1 }
_G.gfx = {
getColor = function()
return color[1], color[2], color[3], color[4]
end,
setColor = function(r, g, b, a)
if type(r) == 'table' then
color = { r[1], r[2], r[3], r[4] }
else
color = { r, g, b, a }
end
end,
print = function() end,
}
end)

teardown(function()
_G.gfx = prev_gfx
end)

describe('write_token', function()
it('leaves the active color untouched', function()
gfx.setColor(0.1, 0.2, 0.3, 1)
ViewUtils.write_token(0, 0, 'x',
{ 1, 0, 0, 1 }, { 0, 0, 0, 1 }, false)
local r, g, b = gfx.getColor()
assert.same({ 0.1, 0.2, 0.3 }, { r, g, b })

ViewUtils.write_token(0, 0, 'x',
{ 1, 0, 0, 1 }, { 0, 0, 0, 1 }, true)
r, g, b = gfx.getColor()
assert.same({ 0.1, 0.2, 0.3 }, { r, g, b })
end)
end)
end)
Loading