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

Nebula markers vectorization #3172

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

10110111
Copy link
Contributor

This is a continuation of #3166, now vectorizing markers of open clusters: simple and with nebulosity.

The number of dots now depends on the size of the marker. The markers of open clusters with nebulosity might look not the best way when they are small. Do you think the points should be moved closer to the center? Or is the way it looks now OK?

Screenshots

Old

stellarium-009

New

stellarium-010

@github-actions
Copy link

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.

@gzotti
Copy link
Member

gzotti commented Apr 11, 2023

Nice! Yes, IMHO the dots in CL+Neb should be a bit contracted. And now that this would technically be possible, can you please try mixing "green" nebula color box with "yellow" cluster color dot circle?

@10110111
Copy link
Contributor Author

can you please try mixing "green" nebula color box with "yellow" cluster color dot circle?

I can, but won't it look as if we have two nebulas of different kinds at the same spot? Or do you want to visually separate the nebulosity as a separate nebula?

@10110111
Copy link
Contributor Author

This is how the color-separated version looks. Maybe the circle doesn't have to be contracted in this case.

stellarium-009

@gzotti
Copy link
Member

gzotti commented Apr 11, 2023

I like the bicolor version. However, I'd still make the circle smaller to keep some buffer around the dots.

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

alex-w commented Apr 12, 2023

The bicolor version of C+N markers is acceptable, but IMHO the circle should be a bit small - does not touche the rectangle.

@Atque
Copy link
Contributor

Atque commented Apr 12, 2023

Shouldn’t the circles also be vectorized?

@10110111
Copy link
Contributor Author

Shouldn’t the circles also be vectorized?

Why do you think they haven't been? Or do you want to also vectorize the fluffy points that make up the circles?

@Atque
Copy link
Contributor

Atque commented Apr 12, 2023

Shouldn’t the circles also be vectorized?

Why do you think they haven't been? Or do you want to also vectorize the fluffy points that make up the circles?

Yes, I would expect that vectorized markers be fully vectorized.

@10110111
Copy link
Contributor Author

Yes, I would expect that vectorized markers be fully vectorized.

I don't find any use in this, only potential performance ramifications.

@gzotti
Copy link
Member

gzotti commented Apr 12, 2023

There is of course a change in overall appearance. The old icons had soft edges which became more blurry by upscaling (mitigated by a few icon sizes), while the new line-based symbols are hard-edged, but the dots are still "fluffy". Maybe the dots should become hard-edged as well? I understand drawing blurred lines needs more GPU code.

@10110111
Copy link
Contributor Author

10110111 commented Apr 12, 2023

However, I'd still make the circle smaller to keep some buffer around the dots

New version:

2
3
4

@10110111
Copy link
Contributor Author

Maybe the dots should become hard-edged as well?

Not sure about this. Some users have multisampling disabled, and such circles will look ugly when aliased.

src/core/modules/Nebula.cpp Outdated Show resolved Hide resolved
@alex-w alex-w self-requested a review April 13, 2023 12:46
@10110111
Copy link
Contributor Author

All the comments seem to have been addressed now.

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.

I did not run the latest changes, but they seem complete now (including manual! :-), thank you!

@10110111 10110111 force-pushed the nebula-markers-vectorization branch from 9a8fc7f to 49869e3 Compare April 13, 2023 23:29
@10110111 10110111 merged commit 74fb2fd into Stellarium:master Apr 13, 2023
@10110111 10110111 deleted the nebula-markers-vectorization branch April 14, 2023 16:19
@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.

4 participants