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

Rounded rectangles: optimize and anti-alias #11498

Merged
merged 2 commits into from Mar 8, 2024

Conversation

zwim
Copy link
Contributor

@zwim zwim commented Feb 27, 2024

This is an optimization which depends on koreader/koreader-base#1738


This change is Reviewable

@zwim zwim marked this pull request as draft February 28, 2024 17:41
@zwim zwim force-pushed the optimize_rounded branch 2 times, most recently from a7a9596 to dbe2931 Compare March 4, 2024 07:48
Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

(Don't forget to bump base with this PR for a self sufficient commit.)

Comment on lines 689 to 690
table.insert(self.menu_items.developer_options.sub_item_table, {
text = _("Anti-alias UI elements"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this out of just in-between the somehow related xtext & rtl mirrorring :) ie. above near the "C blitter" thing, to which "antialias" sounds more related.
(I let @NiLuJe tell if it's worth having this toggable.)

Copy link
Member

@NiLuJe NiLuJe Mar 5, 2024

Choose a reason for hiding this comment

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

It looks to be fairly non-intrusive, so, eh, why not ;).

I'd make the label more precise though, because "UI elements" is kind of an overly optimistic generalization, given that it really only affects rounded corners ;p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is _("Anti-alias rounded corners in UI") good English?

Copy link
Member

Choose a reason for hiding this comment

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

Grammatically sure, but what purpose does "in UI" serve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"in UI" should point out, that only the corners in our UI gets anti-aliased and not something else (like an image ...).

Copy link
Member

Choose a reason for hiding this comment

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

That strikes me as redundant (i.e., what other rounded corners would we even know are such a thing as rounded corners other than those we very explicitly know about because we draw them), but if you prefer. /shrug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong feeling. I can go with you for consistency (as you have texted much more than I ;) )

Comment on lines 695 to 696
local old_val = G_reader_settings:readSetting("anti_alias_ui", 1)
G_reader_settings:saveSetting("anti_alias_ui", 1 - old_val)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you want 1 and 0 so it is passed as-is to the C function. While true and false feels more like what we are used to in frontend - where we can use stuff to toggle/flipNilOrTrue that are better than this ugly 1 - old_val to switch between 1 and 0 :)
If there is only one call, you could just use there anti_alias and 1 or 0 which is how we do it nearly everywhere, ie. in credocument.

@@ -130,10 +135,13 @@ function FrameContainer:paintTo(bb, x, y)
self.inner_bordersize, self.color, self.radius)
end
if self.bordersize > 0 then
local anti_alias = G_reader_settings:readSetting("anti_alias_ui", 1);
require("logger").dbg("anti_alias_ui:", type(anti_alias), anti_alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

logging to be removed :)

@Frenzie Frenzie added this to the 2024.04 milestone Mar 5, 2024
@zwim zwim changed the title Rounded rectangles: don't draw area of border twice Rounded rectangles: optimize and anti-alias Mar 6, 2024
@zwim
Copy link
Contributor Author

zwim commented Mar 6, 2024

Before merging, could you please check if I did the bump base right.

@zwim zwim marked this pull request as ready for review March 6, 2024 16:05
@poire-z
Copy link
Contributor

poire-z commented Mar 6, 2024

The base bump looks fine to me.

Just a space to remove at the end of this line
frontend/apps/filemanager/filemanagermenu.lua:630:64: line contains trailing whitespace

bb:paintBorder(x + self.margin, y + self.margin,
container_width - self.margin * 2,
container_height - self.margin * 2,
self.bordersize, self.color, self.radius)
self.bordersize, self.color, self.radius, anti_alias)
Copy link
Contributor

@poire-z poire-z Mar 6, 2024

Choose a reason for hiding this comment

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

Is it ok to pass a bool to paintBorder, where it will be passed to paintRoundedCorner() as anti_alias or 0, so true or 0 - and then to cblitbuffer.BB_paint_rounded_corner(..., int anti_alias) as anti_alias or 0.
(Not forgetting that in Lua land, the number 0 is true, not false.)
It may work alright, even if a bit non-obvious to grasp, and no need to rebump base - just want somebody else to check all is fine whether we pass initially anti_alias as true of false.

Copy link
Member

Choose a reason for hiding this comment

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

It should be safe, yeah (c.f., Lua -> C for booleans in http://luajit.org/ext_ffi_semantics.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good point.

(Not forgetting that in Lua land, the number 0 is true, not false.)

Yepp and anti_alias=0 or 0 gives 0 -> Which is a noop.

We have to pass a parameter to cblitbuffer.BB_paint_rounded_corner(..., int anti_alias), otherwise we get a runtime error.
In Lua land we want (your proposal) to pass nil, false or true.
If we hand over

  • nil to paintBorder, anti_alias or 0 will expand to 0; in C-land 0 means false;
  • false to paintBorder, anti_alias or 0 will expand to 0; in C-land 0 means false;
  • true to paintBorder, anti_alias or 0 will expand to true; in C-land any value except 0 (most certainly uint 0xffffffff) means true.

If someone will hand over (in future)

  • 0 to paintBorder, anti_alias or 0 will expand to 0; in C-land 0 means false;
  • 5 to paintBorder, anti_alias or 0 will expand to 5; in C-land any value except 0 means true.

Of course, we can make all those considerations explicit, but then we have to change the blitbuffer API.
Keeping the proposed API means any value except 0 or false enables anti-aliasing. (And maybe in future different kinds of anti-aliasing ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NiLuJe You are to fast for me ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your double checkings :)

@zwim
Copy link
Contributor Author

zwim commented Mar 6, 2024

In case someone wants to commit this PR, please don't squash.

@zwim zwim merged commit c8f39c3 into koreader:master Mar 8, 2024
2 of 3 checks passed
@zwim zwim deleted the optimize_rounded branch March 8, 2024 07:00
@poire-z
Copy link
Contributor

poire-z commented Mar 8, 2024

(Use "rebase" instead of "merge commit" next time, just to avoid a useless commit - still wondering what's the use for this stupid "merge commit".)

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

Successfully merging this pull request may close these issues.

None yet

4 participants