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

Vectorize markers of diffuse and dark nebulas #3166

Merged
merged 2 commits into from
Apr 10, 2023

Conversation

10110111
Copy link
Contributor

@10110111 10110111 commented Apr 9, 2023

Description

Currently the nebular markers are made of a hierarchy of similar rectangles differing only by width of their lines. This is ugly technically, and it doesn't look nice when the nebula has large size on the screen.

This PR replaces the texture-based diffuse (and now also dark) nebula markers with a line-based scalable one. I tried to make it look mostly the same as the old version, just less pixelized, and without the fancy gradient (this is for simplicity and performance; tell me if you dislike it).

Screenshots

Old

stellarium-009

New

stellarium-010

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update

How Has This Been Tested?

Test Configuration:

  • Operating system: Ubuntu 20.04
  • Graphics Card: Intel UHD Graphics 620

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@github-actions
Copy link

github-actions bot commented Apr 9, 2023

Great PR! Please pay attention to the following items before merging:

Files matching src/**/*.cpp:

  • Are possibly unused includes removed?

This is an automatically generated QA checklist based on modified files.

@alex-w alex-w added this to the 23.2 milestone Apr 9, 2023
@gzotti
Copy link
Member

gzotti commented Apr 9, 2023

Yes, very welcome! I did not test performance, though. Any change you can see?
I still remember changing from generic DSO blob to type icons, and later doing a "preliminary upscale, until s.o. can vectorize it...".

src/core/modules/Nebula.cpp Outdated Show resolved Hide resolved
@10110111
Copy link
Contributor Author

10110111 commented Apr 9, 2023

I did not test performance, though. Any change you can see?

No difference on my machine.

@10110111
Copy link
Contributor Author

10110111 commented Apr 9, 2023

So, this is how it looks now after removing the hack that reduced marker size:

stellarium-009

@10110111 10110111 force-pushed the nebula-markers-vectorization branch from 309e3f5 to 705a112 Compare April 9, 2023 18:02
@10110111 10110111 changed the title Vectorize markers of diffuse nebulas Vectorize markers of diffuse and dark nebulas Apr 9, 2023
@10110111
Copy link
Contributor Author

10110111 commented Apr 9, 2023

I've also changed the marker of the dark nebulas, the result is as follows.

Old

stellarium-009

New

stellarium-010

@alex-w
Copy link
Member

alex-w commented Apr 9, 2023

Good!

@10110111 10110111 force-pushed the nebula-markers-vectorization branch from 5e1799d to fd4ef34 Compare April 9, 2023 20:23
@gzotti
Copy link
Member

gzotti commented Apr 9, 2023

Tested on RPi3 for the same sky part: no speed penalty. It even may be faster in the hundredths (seen typically 4.88 for master vs 4.91 for this, but both values were fluctuating by ±0.2 ... no, cannot really say which is faster).

@gzotti
Copy link
Member

gzotti commented Apr 10, 2023

Do you change all icons? Globulars are also multi-resolution, and galaxies may want that as well. Open clusters may need a very different method for the multi-dots. Maybe make separate private drawDiffuseNebula(pos, size, ...) etc. methods for them (also to combine "cluster with nebulosity" - this could even become two-colored), codefactor.io already complains about "very complex method", and it can only get longer.

@10110111
Copy link
Contributor Author

Do you change all icons?

No, I only changed two types of nebulas: the rounded-rectish ones. I intend to work on others separately, since they might require more work (especially the dotted ones, where simple lines might not look nice enough).

Copy link
Member

@gzotti gzotti left a comment

Choose a reason for hiding this comment

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

It looks good and works well, thanks. If you want to change others later, I can approve it now.
It would be wise to - at least in the next round - refactor a bit and make separate drawing methods to avoid too complex code.

@10110111
Copy link
Contributor Author

It would be wise to - at least in the next round - refactor a bit and make separate drawing methods to avoid too complex code.

Yes, I did think of this, but decided that I'll first make all the meat and see what common parts can be combined and extracted, and then decide how exactly to refactor.

@10110111 10110111 merged commit 81e7172 into Stellarium:master Apr 10, 2023
@10110111 10110111 deleted the nebula-markers-vectorization branch April 10, 2023 10:43
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Apr 19, 2023
@github-actions
Copy link

Hello @10110111!

Please check the fresh version (development snapshot) of Stellarium:
https://github.com/Stellarium/stellarium-data/releases/tag/weekly-snapshot

@alex-w alex-w removed the state: published The fix has been published for testing in weekly binary package label Jul 2, 2023
@github-actions
Copy link

github-actions bot commented Jul 2, 2023

Hello @10110111!

Please check the latest stable version of Stellarium:
https://github.com/Stellarium/stellarium/releases/latest

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

Successfully merging this pull request may close these issues.

3 participants