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.

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

Jump to bottom

Issue 290 export to SVG #588

Merged
merged 5 commits into from
Jun 2, 2021
Merged

Issue 290 export to SVG #588

merged 5 commits into from
Jun 2, 2021

Conversation

serk12
Copy link
Collaborator

@serk12 serk12 commented May 25, 2021

I've added the SVG "ball and stick" export button. following #290 issue.
Some important notes are:

  • Installing the svg-qt libs (libqt5svg5-dev)
  • The frustrumCulling function is too conservative and I don't know exactly why

@github-actions
Copy link
Contributor

Here are the build results
macOS.dmg
Ubuntu1804.tar.gz
Ubuntu2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

We use sign-off on our commits - see:
https://github.com/OpenChemistry/avogadrolibs/pull/588/checks?check_run_id=2664267277

ghutchis
ghutchis requested changes May 25, 2021
View reviewed changes
Copy link
Member

@ghutchis ghutchis left a comment

Choose a reason for hiding this comment

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

Overall, it looks good, although my personal preference is no background color. 😄

I'm trying to figure out if there's a high-res Mac issue because of the scaling ratio. I'll post a some examples.

avogadro/qtplugins/svg/svg.cpp Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.cpp Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.cpp Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.cpp Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.cpp Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.cpp Show resolved Hide resolved
avogadro/qtplugins/svg/svg.h Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.h Outdated Show resolved Hide resolved
avogadro/qtplugins/svg/svg.h Outdated Show resolved Hide resolved
@ghutchis
Copy link
Member

Yeah, it's not a resolution / scaling issue - it seems to just be aggressive culling
caffeine.svg.gz
caffeine2.svg.gz

@serk12
feat: first svg exporter
68839c8 
Signed-off-by: serk12 <marc.prat.maso@estudiantat.upc.edu>
@serk12
fix frustum culling using magic number and bound coordinates
b547da3 
Signed-off-by: serk12 <marc.prat.maso@estudiantat.upc.edu>
@serk12
correct occlusion + classic frustum + fix scale + factor
9a9b957 
Signed-off-by: serk12 <marc.prat.maso@estudiantat.upc.edu>
@serk12
renaming, setOptions template, and fix proportion with camera project
2df232c 
Signed-off-by: serk12 <marc.prat.maso@estudiantat.upc.edu>
@serk12
Copy link
Collaborator Author

serk12 commented May 26, 2021

I think we have a OS problem on the m_scene->backgroundColor() function.
In my Ubuntu it returning alpha meanwhile your Mac is returning turquoise, a good choice but not as expected.
cafe.zip

@ghutchis
Copy link
Member

I think we have a OS problem on the m_scene->backgroundColor() function.
In my Ubuntu it returning alpha meanwhile your Mac is returning turquoise, a good choice but not as expected.

No, that's the background color I set. 😄

@ghutchis
Copy link
Member

Code looks much better - @cryos do you want to take a look too?

@ghutchis ghutchis requested review from cryos and ghutchis May 26, 2021 20:53
@github-actions
Copy link
Contributor

Here are the build results
macOS.dmg
Ubuntu1804.tar.gz
Ubuntu2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

To make this clear, this fixes #290

cryos
cryos reviewed May 28, 2021
View reviewed changes
avogadro/qtplugins/svg/CMakeLists.txt Outdated
""
)

target_link_libraries(SVG LINK_PRIVATE Qt5::Svg)
Copy link
Member

Choose a reason for hiding this comment

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

Should combine into a single line

Copy link
Member

Choose a reason for hiding this comment

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

I think pretty much all the plugins have been using multiple lines here, so let's work on that in a separate change.

cryos
cryos reviewed May 28, 2021
View reviewed changes
avogadro/qtplugins/svg/svg.cpp
@@ -0,0 +1,277 @@
#include "svg.h"
Copy link
Member

Choose a reason for hiding this comment

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

Missing copyright header, consider using the short one in array.h I think

cryos
cryos reviewed May 28, 2021
View reviewed changes
avogadro/qtplugins/svg/svg.h
@@ -0,0 +1,102 @@
/******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

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

Consider the short copyright header without name or year.

cryos
cryos reviewed May 28, 2021
View reviewed changes
Copy link
Member

@cryos cryos left a comment

Choose a reason for hiding this comment

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

I need to head out, but took a quick look, haven't had a chance to build/play with it yet. It looks cool, I pointed out a few minor tweaks that you could make. Look forward to playing with it more next week when I have time in front of a compiler.

@ghutchis ghutchis added the enhancement feature changes / API changes label May 28, 2021
@ghutchis
Copy link
Member

Just wanted to mention that it looks good to go. I wanted to wait for @cryos on this one, and it's a holiday weekend here in the States.

@serk12
copyright and digested svg cmake
b850d75 
Signed-off-by: serk12 <marc.prat.maso@estudiantat.upc.edu>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2021

Here are the build results
Ubuntu1804.tar.gz
Ubuntu2004.tar.gz
Win64.exe
Artifacts will only be retained for 90 days.

@ghutchis
Copy link
Member

ghutchis commented Jun 1, 2021

The Mac build is on me - I'll take a look after this is merged.

@ghutchis ghutchis merged commit 53db2a0 into OpenChemistry:master Jun 2, 2021
@serk12 serk12 deleted the issue-290 branch June 7, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghutchis ghutchis ghutchis approved these changes

@cryos cryos cryos left review comments

Assignees
No one assigned
Labels
enhancement feature changes / API changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@serk12 @ghutchis @cryos