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

Allow line loops on Cylindrical projection. #3505

Merged
merged 3 commits into from
Nov 11, 2023
Merged

Allow line loops on Cylindrical projection. #3505

merged 3 commits into from
Nov 11, 2023

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Nov 11, 2023 •
edited
Loading

Description

This prevents the connection of line segments over disconnected viewport edges.

For this, I convert the LineLoop to an indexed set of GL_LINEs.

Fixes #3503 (issue)

Screenshots (if appropriate):

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
  • Housekeeping

How Has This Been Tested?

Disable lunar scale-up, activate earth shadow circles (GridLinesMgr). Zoom out, place earth's shadow cycles close to screen edge.

Test Configuration:

  • Operating system: Windows11
  • Graphics Card: Geforce, but irrelevant

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

@gzotti gzotti added the importance: medium A bit annoying, minor miscalculation, but no crash label Nov 11, 2023
@gzotti gzotti added this to the 23.4 milestone Nov 11, 2023
@gzotti gzotti self-assigned this Nov 11, 2023
@github-actions github-actions bot requested review from 10110111 and alex-w November 11, 2023 03:07
@github-actions 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.

10110111
10110111 reviewed Nov 11, 2023
View reviewed changes
Copy link
Contributor

@10110111 10110111 left a comment

Choose a reason for hiding this comment

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

Line 126 changes primitive type to triangles, which is what you've missed.

@gzotti gzotti force-pushed the fix/earth-shadow-circles branch from 9d63a4b to 44c4b61 Compare November 11, 2023 09:19
@gzotti gzotti marked this pull request as ready for review November 11, 2023 09:22
@github-actions github-actions bot requested a review from 10110111 November 11, 2023 09:22
@Atque
Copy link
Contributor

Atque commented Nov 11, 2023

Great! But the bug is still present with umbra/penumbra for satellites.

10110111
10110111 reviewed Nov 11, 2023
View reviewed changes
src/core/StelVertexArray.cpp Outdated
@@ -55,14 +56,13 @@ StelVertexArray StelVertexArray::removeDiscontinuousTriangles(const StelProjecto
else
{
ret.indices.clear();
unsigned short int limit;
const unsigned short int limit=static_cast<unsigned short int>(vertex.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be a good idea to assert that vertex.size() <= USHRT_MAX (or std::numeric_limits<unsigned short>::max()).

Also, this line could be shortened by removing repetition from the LHS and the useless int from the RHS. Or just using an implicit cast.

10110111
10110111 reviewed Nov 11, 2023
View reviewed changes
src/core/StelVertexArray.cpp Outdated
@@ -23,6 +23,7 @@
StelVertexArray StelVertexArray::removeDiscontinuousTriangles(const StelProjector* prj) const
{
StelVertexArray ret = *this;
ret.primitiveType = Triangles;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to put it inside the switch, for symmetry with lines.

10110111
10110111 approved these changes Nov 11, 2023
View reviewed changes
Copy link
Contributor

@10110111 10110111 left a comment

Choose a reason for hiding this comment

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

Other than my comments, LGTM.

@gzotti gzotti merged commit 44f3354 into master Nov 11, 2023
16 checks passed
@gzotti gzotti deleted the fix/earth-shadow-circles branch November 11, 2023 11:27
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Nov 12, 2023
@github-actions GitHub Actions
Copy link

Hello @gzotti!

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 Dec 23, 2023
@github-actions GitHub Actions
Copy link

Hello @gzotti!

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
Reviewers

@10110111 10110111 10110111 approved these changes

@alex-w alex-w Awaiting requested review from alex-w

Assignees

@gzotti gzotti

Labels
importance: medium A bit annoying, minor miscalculation, but no crash
Milestone
23.4
Development

Successfully merging this pull request may close these issues.

Eclipse shadow markers ugly in certain projections
4 participants
@gzotti @Atque @10110111 @alex-w