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

Loading SVG files for Icons (annotations). #10104

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

Conversation

achary
Copy link
Contributor

@achary achary commented Jan 25, 2023

SVGs are permitted image types (as per Modelica specification). Support for SVG as a valid image type usable in annotation is added.

Related Issues

#10102

Purpose

as in the issue.

Approach

  1. Adding support for new file type (extension).
  2. Checking base64 encoded content (in case image is stored inline in the icon annotation itself) does represent the original SVG file, and not a rendered pixmap.
    The app decides, based on the decoded base64 content, whether it deals with one of binary formats (PNG, JPG) that can be simply pushed to QPixmap and alike, without worrying about them, or whether it's SVG and needs slightly different rendering path. For that it constructs one of appropriate image renderers.
  3. This in turn, ensures that SVG images are rendered on demand according to their actual display size, so we get a good visual effect - no zoomed pixels, no edge blurs.

This PR where it stands, makes SVG files first-class citizens and improves quality of their renderings in the UI (can be zoomed in without pixelization).

Screens

The SVG file doc/images/logo.svg was used here:

image

and with larger zoom:

image

@@ -53,6 +53,7 @@ QString Helper::omnotebookFileTypes = "OMNotebook Files (*.onb *.onbz *.nb)";
QString Helper::ngspiceNetlistFileTypes = "ngspice Netlist Files (*.cir *.sp *.spice)";
QString Helper::imageFileTypes = "SVG (*.svg);;PNG image (*.png);;Windows BMP image (*.bmp);;TIFF (*.tiff)";
QString Helper::bitmapFileTypes = "Image Files (*.png *.bmp *.jpg *.jpeg);;PNG image (*.png);;Windows BMP image (*.bmp);;JPEG (*.jpg *.jpeg)";
Copy link
Contributor Author

@achary achary Jan 25, 2023

Choose a reason for hiding this comment

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

FYI: the "bitmapFileTypes" image filter has its usage for selecting bitmap textures somewhere in 3D animation viewer, so not extending this one with SVG as it's not the intention of this PR to alter this accidentally.

@adeas31 adeas31 self-requested a review January 26, 2023 07:39
@achary
Copy link
Contributor Author

achary commented Jan 26, 2023

@casella, are there any formatting standards you stick to in this project?
I've only found info about trailing whitespaces in the
How about intentation, max line length, ordering includes, brackets etc?
Clang-format etc? no?

Writing because my editor settings (almost) by default makes plenty of changes (like include order sorting/grouping). Are you accepting such formatting changes mixed with functional changes? What's your policy here?

Sorry for plenty of question at once. Better to clarify upfront...

@adeas31
Copy link
Member

adeas31 commented Jan 26, 2023

@achary read this https://github.com/OpenModelica/OpenModelica/tree/master/OMEdit#coding-style

For includes we first include our own files and then the Qt headers.
There are no standard settings for max line length. For brackets, use the standard GNU style.

@achary
Copy link
Contributor Author

achary commented Jan 26, 2023

@adeas31 Thanks. I think it only scratches the surface, but I'll try to stick.

@casella
Copy link
Contributor

casella commented Jan 27, 2023

@achary I guess you can also get an idea by looking at the existing codebase.


painter->drawImage(target, mImage.mirrored());
Copy link
Contributor Author

@achary achary Jan 27, 2023

Choose a reason for hiding this comment

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

FYI: This is the root place where the change actually needed to be applied at. Instead of always drawing a bitmap image, now, we now delegate the painting operation to whatever the mpRenderer (runtime polymorphic type) has been constructed. Bitmap images render pretty much the same way (see BitmapRenderer class) and SVG images use QSvgRenderer under the hood instead (see `SvgRenderer class).

@achary
Copy link
Contributor Author

achary commented Jan 27, 2023

The change ended up being slightly more involving than I've imagined prior seeing the code.
There was a bit of refactoring made of the ShapeAnnotation and BitmapAnnotation classes.

IMO the ShapeAnnotation class is much waaaay too heavy, as it contains data fields for all possible occasions (like for instance font details, which apply only to TextAnnotation only etc.) - worth considering refactoring that too in the future.

SVGs are permitted image types (as per modelica specification).
Support for SVG as a valid image type usable in annotation is added.
@achary achary changed the title Loading SVG files for Icons (annotations) added. Loading SVG files for Icons (annotations). Jan 27, 2023
@achary
Copy link
Contributor Author

achary commented Jan 28, 2023

BTW: Some CI on Jenkins jobs looks unstable, giving false negatives.

The job testsuite-matlab-translator gave two different results (first fail, then pass) on two different runs over the same code (a trivially amended Git commit).

@achary achary marked this pull request as ready for review January 28, 2023 10:30
@casella
Copy link
Contributor

casella commented Jan 28, 2023

@adeas31 can you please review and merge if you think this is OK? Thanks!

@sjoelund
Copy link
Member

BTW: Some CI on Jenkins jobs looks unstable, giving false negatives.

The job testsuite-matlab-translator gave two different results (first fail, then pass) on two different runs over the same code (a trivially amended Git commit).

You need to check the logs to see what is wrong:

ryzen-5950x-2-3 does not seem to be running inside a container
$ docker run -t -d -u 1001:1001 -w /var/lib/jenkins3/ws/OpenModelica_PR-10104 -v /var/lib/jenkins3/ws/OpenModelica_PR-10104:/var/lib/jenkins3/ws/OpenModelica_PR-10104:rw,z -v /var/lib/jenkins3/ws/OpenModelica_PR-10104_tmp:/var/lib/jenkins3/ws/OpenModelica_PR-10104_tmp:rw,z -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** -e ******** docker.openmodelica.org/build-deps:v1.16.3 cat
 ---> Running in 685d52f784a9
ERROR: Timeout after 180 seconds

Sometimes jobs simply timeout... Or crash inside of Jenkins itself and it has nothing to do with the code. Sometimes it's GitHub that is slow.

Copy link
Member

@adeas31 adeas31 left a comment

Choose a reason for hiding this comment

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

I didn't look at the implementation details yet.

I compiled the code and it is not working as expected. For example, look at Modelica.Mechanics.MultiBody.Examples.Systems.RobotR3.FullRobot. Also create a new model and try adding a bitmap to it.

@adeas31
Copy link
Member

adeas31 commented Feb 7, 2023

FYI I also get message,

QIODevice::read (QFile, "modelica:\Modelica\Resources\Images\Mechanics\MultiBody\Examples\Systems\RobotR3\robot_kr15.png"): device not open

@achary
Copy link
Contributor Author

achary commented Feb 12, 2023

Oh, that's worrying. I'll see if I can reproduce it.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

5 participants