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

Add image-scaling property to Images to control how they are scaled #465

Merged
merged 5 commits into from
Sep 6, 2021

Conversation

ryban
Copy link
Contributor

@ryban ryban commented Sep 4, 2021

Like CSS image-rendering it has "smooth" and "pixelated" options. Only OpenGL has been tested right now, have not tested WASM or Qt. Right now the first instance of a @image-url() will set the scaling for that specific image. The same image used from memory on the otherhand can all have a different scaling property.

This example shows scaling a 8x8 pixel image

Still need to update documentation, but I wanted to get other backends working, possibly fix the image-url issue, and ask if there is a better way to implement this first.

Like CSS image-rendering it has "smooth" and "pixelated" options.
Only OpenGL has been tested right now, have not tested WASM or Qt.
Right now the first instance of a @image-url() will set the scaling
for that specific image. The same image used from memory on the otherhand
can all have a different scaling property.
@CLAassistant
Copy link

CLAassistant commented Sep 4, 2021

CLA assistant check
All committers have signed the CLA.

@ogoffart
Copy link
Member

ogoffart commented Sep 5, 2021

Thanks for your contribution, this is very welcome.

It looks good to me, I just have a few minor point:

  • The CI is failing because you added the example in Cargo.toml, but you forgot to git add the examples. I don't think there should be a rust example for that, but if it is just a .60 file it can be added in tests/cases/examples
  • it is missing the Qt backend implementation. It should be trivial to add as well (just add the right flag to the painter) But i'd understand if you don't have Qt and i can just add this myself
  • As you said, it's missing documentation and changelog entry.

Since the wasm backend is the same as the GL backend, i imagine it should work out of the box.

Then, there is the question of naming.
With CSS, it is called image-rendering https://developer.mozilla.org/en-US/docs/Web/CSS/image-rendering
I think it might make more sense to keep the same name as CSS. Maybe @tronical will have an opinion the name as well.

@ryban
Copy link
Contributor Author

ryban commented Sep 5, 2021

Yeah, I forgot to remove the example from Cargo.toml because the example was not really interesting. I don't have Qt so I hadn't implemented it, but if someone else can implement it that would help. I did get wasm to compile after working through a perl issue and it does work as expected.

I chose image-scaling instead of image-rendering to be more direct on what the setting is doing, but thinking about it image-scaling could be misinterpreted as a scaling factor. Plus there is value in keeping with what people know.

@tronical
Copy link
Member

tronical commented Sep 5, 2021

I very much like the name as proposed in the latest patch set :-) (image-rendering). Nice addition!

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Only thing is to fix CI because the test has an error, and one should do cargo fmt

This file is also available under commercial licensing terms.
Please contact info@sixtyfps.io for more information.
LICENSE END */
import { HorizontalLayout } from "sixtyfps_widgets.60";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { HorizontalLayout } from "sixtyfps_widgets.60";

Not needed because HorizontalLayout is a builtin element

@ogoffart ogoffart merged commit e7245cd into slint-ui:master Sep 6, 2021
@ogoffart
Copy link
Member

ogoffart commented Sep 6, 2021

Thank you!

I'll do the Qt backend changes now

ogoffart added a commit that referenced this pull request Sep 6, 2021
@ogoffart
Copy link
Member

ogoffart commented Sep 6, 2021

I done the Qt backend.

Btw, i noticed a bug: the image-rendering property is not properly applied if both image are the same file with the GL backend, because the cached texture contains the image-rendering flag, but it is not part of the cache key.

tronical added a commit that referenced this pull request Sep 6, 2021
…ge-rendering properties

As tests/cases/examples/image_rendering.60 itself demonstrates, the png file
should be loaded into the CPU side image cache once, but the two textures
should be cached independently.

cc #465
tronical added a commit that referenced this pull request Sep 6, 2021
…ge-rendering properties

As tests/cases/examples/image_rendering.60 itself demonstrates, the png file
should be loaded into the CPU side image cache once, but the two textures
should be cached independently.

cc #465
@ryban
Copy link
Contributor Author

ryban commented Sep 6, 2021

Thanks for merging and adding Qt support. Yeah, which ever Image instance references a file first "locks in" the image-rendering setting for that image. I wasn't sure how to change the cache key.

@ryban ryban deleted the image-scaling branch September 6, 2021 20:11
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.

4 participants