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

Projection: plate carree #2856

Merged
merged 15 commits into from
Dec 10, 2022
Merged

Projection: plate carree #2856

merged 15 commits into from
Dec 10, 2022

Conversation

gzotti
Copy link
Member

@gzotti gzotti commented Nov 18, 2022

Description

Ruslan's shader implementation of Milky Way and ZL makes this finally interesting: A window-filling all-sky map. I can imagine an application using Spout where the result goes into another program, as live texture for some object, or just a printed all-sky map.

Fixes #1120

What currently still looks bad at the seam are of course all meshes which are projected before being sent to the GPU:

  • landscapes
  • constellation artwork
  • DSO textures
  • DSS
  • HiPS
  • sky images (from scripting)
  • An axis-aligned grid has flickering parts on the window edges.
  • Points like NCP flicker as they should be plotted on the edge but may be projected to the other side by rounding.

Fixing the meshes is not part of this PR. For the time being the application is limited to mostly MW/ZL all-sky maps.

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

How Has This Been Tested?

Test Configuration:

  • Operating system: Windows 10
  • Graphics Card: Geforce 960M

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 feature Entirely new feature label Nov 18, 2022
@gzotti gzotti added this to the 1.2 milestone Nov 18, 2022
@gzotti gzotti requested review from alex-w and 10110111 November 18, 2022 15:27
@gzotti gzotti self-assigned this Nov 18, 2022
@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 Author

gzotti commented Nov 20, 2022

I think it's ready for discussion. What I see:

  • great for All-sky maps including just MilkyWay/ZL. Will become also great for horizontal maps when @10110111 (?) can implement shader solutions for the landscapes and other large textured regions. (Not part of this PR.)
  • With the respective grids activated, grids flicker at the screen edge. This could be remedied by plotting all 4 specific straight lines if this projection is active. Necessary?
  • Draw extra gridlines along screen edges
  • Ignore
  • NCP flickers at the edge in alt-azimuthal mode. Solutions:
  • Repeat drawing with position set manually to the left edge.
  • Ignore

@gzotti gzotti marked this pull request as ready for review November 20, 2022 15:00
@10110111
Copy link
Contributor

10110111 commented Nov 21, 2022

Current state of your branch looks like below on my machine. Screen scaling factor is 1.5, if it matters, Qt6 (manually compiled) on Ubuntu 20.04.

Screenshot_2022-11-21_13-48-22

@gzotti
Copy link
Member Author

gzotti commented Nov 21, 2022

OK, yes, scale obviously matters...
Uh, did landscape drop below horizon as well?

@10110111
Copy link
Contributor

10110111 commented Nov 21, 2022

Uh, did landscape drop below horizon as well?

They appear to be at different scales: as I pan around, the horizon of the landscape moves as if with a parallax relative to the horizon of the sky dome.

@10110111
Copy link
Contributor

10110111 commented Nov 21, 2022

Actually, no, it's the atmosphere horizon that moves. The stars move synchronously with the landscape during panning.

@gzotti
Copy link
Member Author

gzotti commented Nov 21, 2022

Grmpf... I have now set 125% and 150% scaling on Windows (works on the fly only with built-in screen), and see no issues (apart from obvious lack-of-screen space issues). For whatever reasons, it crashes with 175% when calling the view settings dialog.

@10110111
Copy link
Contributor

works on the fly only with built-in screen

Change on the fly is done purely by the OS. Qt doesn't get a chance to adapt for some reason. Restart of the app should make Qt aware of the change and in charge of it.

@gzotti
Copy link
Member Author

gzotti commented Nov 21, 2022

Yes, of course I quit Stellarium, changed resolution, and restarted Stellarium. It comes up with the new resolution/bigger GUI but still fills the window. So this is an issue visible only if running on Linux. Getting this screen scaling right on all platforms is a nightmare.

gzotti added 13 commits December 9, 2022 11:09
- ProjectionCylinderFill (derived from ProjectionCylinder)
- No functional changes yet
- also clarified declaration of virtual methods
- Follow Qt's recommendation to code style (Q_DECL_OVERRIDE -> override)
- implementation of 3 methods in base class
- Removed corresponding overrides in several derived classes
- restore customized screen stretch when going to another projection
- better specific map layout
@gzotti gzotti force-pushed the projection_plate_carree branch from eb42335 to e327468 Compare December 9, 2022 10:11
@gzotti
Copy link
Member Author

gzotti commented Dec 9, 2022

@10110111 just tried on Ubuntu 22.04.1/Geforce M 580. I set scaling to 125%, and now I see scaling issues in fullscreen, but for all projections. Our view frame is confined to the upper left quadrant. These issues are also present in master!

Is the problem that you see above limited to the new projection?

@10110111
Copy link
Contributor

10110111 commented Dec 9, 2022

Master works perfectly fine for me (aside from failure to compile due to GregorianCalendar includes being commented out in JKV calendars).

How do you set the scaling factor? I just use the following environment variables:

QT_FONT_DPI=96
QT_SCREEN_SCALE_FACTORS=1.25

Similarly fine it is when I don't use any variables, while the screen DPI is set to 167 (in this case Qt sets scale factor 167/96, i.e. 1.73958...).

I'm testing with Qt 6.4.0 on Ubuntu 20.04 and Intel UHD Graphics 620.

@gzotti
Copy link
Member Author

gzotti commented Dec 9, 2022

Ah no, I set scaling in the OS's Settings GUI. (Ubuntu 22.04.1) First, enable Fractional Scaling, then set to 125%, then (to be sure all programs get it) logout/login. U22.04 includes Qt6.2.4.
I can try with the QT env. variables as keep the OS scaling at 100%, but it is probably not what most users would do.

@10110111
Copy link
Contributor

10110111 commented Dec 9, 2022

Ah no, I set scaling in the OS's Settings GUI.

Oh well, I don't know what tricks they use. Could you post output of xrandr --verbose after having set fractional scaling?

@gzotti
Copy link
Member Author

gzotti commented Dec 9, 2022

xrandr_100pct.txt
xrandr_125pct.txt

This is without the QT... variables set.

@10110111
Copy link
Contributor

10110111 commented Dec 9, 2022

AFAICT, what you are setting is a scaling, but not the high-DPI scaling. Your setting only enlarges the desktop, downsampling the resulting raster to fit on the screen. Globally, regardless of app awareness, in a raster-downsample mode! This is definitely not something we can support well.

Still, I have tried this on my Intel-based machine, and it works without confining the viewport.

From the output names I figure that you are using nvidia driver, right? Generally this is the correct choice, but historically, nvidia's implementation of XRandR was not the best, so the viewport confinement issue you're seeing might be due to a bug there.

Finally, you should be able to reproduce the problem of this branch being messed up while master looks OK by using the environment variable approach (resetting XRandR scaling factor to 100%).

Is the problem that you see above limited to the new projection?

Just retested, the problem equally affects all projections in this branch.

@gzotti
Copy link
Member Author

gzotti commented Dec 9, 2022

AFAICT, what you are setting is a scaling, but not the high-DPI scaling. Your setting only enlarges the desktop, downsampling the resulting raster to fit on the screen. Globally, regardless of app awareness, in a raster-downsample mode! This is definitely not something we can support well.

OK, but I have no HiDPI device. This is what the user sees.

Still, I have tried this on my Intel-based machine, and it works without confining the viewport.

From the output names I figure that you are using nvidia driver, right? Generally this is the correct choice, but historically, nvidia's implementation of XRandR was not the best, so the viewport confinement issue you're seeing might be due to a bug there.

Ah, OK. Yes, this is the NVidia driver for Ubuntu.

Finally, you should be able to reproduce the problem of this branch being messed up while master looks OK by using the environment variable approach (resetting XRandR scaling factor to 100%).

Is the problem that you see above limited to the new projection?

Just retested, the problem equally affects all projections in this branch.

Interesting. Changes other than the new projection (variant) were not so many. Let's see...

@gzotti
Copy link
Member Author

gzotti commented Dec 9, 2022

Interesting. With these variables and without setting scaling via Windows system settings, I see the issue on Windows. Is this what users are supposed to configure, or is this a weird hack which should look broken by design?

@10110111
Copy link
Contributor

Is this what users are supposed to configure, or is this a weird hack which should look broken by design?

These variables have two use cases:

  • for the developers to test the GUI on different scaling factors,
  • for the users to override wrong system configuration (e.g. wrong EDID reported by a monitor).

They definitely aren't supposed to look broken.

@10110111
Copy link
Contributor

e624ccd indeed makes things work better.

To make this simplification work, the Vector4<T>::operator*(T) should have a custom template parameter to take right-hand operand as is, without coercing it to T. (Ideally, all these overloaded operators should not force T for scalars.)

@gzotti
Copy link
Member Author

gzotti commented Dec 10, 2022

Yes, I have concluded the same. But it's not needed for this PR. So, I think this should be ready to merge.

The next big thing would be moving some drawing code for the Landscapes into shader code to avoid the visible gaps. For now, this new mode works best for all-sky maps without labels or lines along the edge.

About HiDPI, as they are becoming more common, we should add a section to the User Guide, e.g. 2.5.1.

@10110111
Copy link
Contributor

The next big thing would be moving some drawing code for the Landscapes into shader code to avoid the visible gaps.

I have this in progress (spherical is done, fisheye almost done, old style not started yet), but I suppose it won't get into 1.2, since I'm getting a new job next week and won't have as much time then.

@gzotti
Copy link
Member Author

gzotti commented Dec 10, 2022

Great to read, and of course this can wait until 23.1 or later. Congrats on the new job!
@alex-w any comments/objections? Or can we merge this?

@gzotti gzotti merged commit fd8cf36 into master Dec 10, 2022
@gzotti gzotti deleted the projection_plate_carree branch December 10, 2022 11:50
@alex-w alex-w added the state: published The fix has been published for testing in weekly binary package label Dec 12, 2022
@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 25, 2022
@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
Labels
feature Entirely new feature
Development

Successfully merging this pull request may close these issues.

Add equirectangular projection
3 participants