-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update dep version requirements: research-app 0.7, reproject 0.8 #313
Conversation
…ually passes This will make it easier to isolate image fails in the artifacts.
Current macOS has some outputs that look like we need even longer timeouts, but I'd like to have some more data about the timing before trying to bump things up.
@Carifio24 Here's a mystery for you. The Windows and macOS builds are now failing with, among other things, Qt tests where the imagery seems to be shifted upwards from where it should be. (There are other failures, but they seem to be more reasonable.) I can't reproduce locally with a build on my Windows partition. Any ideas about why we might be getting this failure mode? I've been trying to see whether using true OpenGL vs. software rasterization makes a difference (that's one thing that distinguishes the Windows and macOS Azure Pipelines builds), but that's its own can of worms ... |
If I'm analyzing this correctly, the strange vertical offsets don't happen when using the 0.6 version of the research app, and they don't happen when using Qt 5.15 on Windows (vs. Conda's 5.12), and they don't happen when using real GL as opposed to the software renderer. |
966a90d
to
773ef0f
Compare
OK, I tried changing the CI scripts to use Qt 5.15 (by installing with pip), and on macOS the vertical offset is still there, so that's not helpful. (On Windows, other issues cropped up.) |
When I temporarily changed the package to use version 0.6 of the research app, the problem was fixed. (That's Pipelines build 20210922.2). |
On my Windows machine, if I resize the physical WWT window, the offset seems to be stable regardless of what I change, so it seems like the view center is really shifted, and it's probably not an issue of the output bitmap getting cropped oddly or something. |
In the update: In |
I got it to the point where I could bisect on different versions of the engine on my Windows partition. Not too surprisingly, the culprit seems to be somewhere in WorldWideTelescope/wwt-webgl-engine#115, the tiled FITS pull request. |
@imbasimba This pull request upgrades pywwt to use the latest version of the WWT research app and engine, which turns out to cause new failures in the CI test suite. I believe that I've traced this down to the WebGL 1 fallback code: e.g., the pywwt test Would you be able to look into this and see if you can confirm my analysis? This version of pywwt isn't using the tiled fits functionality, so it would be something with the WebGL1 approach to SkyImage FITS images. update: OK, yeah, if I hack my pywwt to use a local copy of the research-app/engine, and hack that local copy to always set UseGlVersion2 to False, I can reproduce the offset positioning on my Linux machine. |
Hmm... I'll look into this |
Here is the little thing I messed up in the SkyImage/TangentTile class merge. |
Thanks for investigating! My local testing looks like it confirms that you've found the problem. |
This gets the WebGL 1 FITS SkyImage positioning fix that should position us for a passing CI. We'll still need to update the reference image corpus, but hopefully this should solve the egregious failures.
The new WebGL2 FITS rendering code and imageset zoom setup alters some of our expected outputs. Note in particular that in the `image_layer_gal` test, the WebGL1 and WebGL2 renderers produce fairly different opacity effects. As best I can tell, they are both behaving as intended, it's just that the shaders (TileShader and FitsShader) approach opacity in different ways. It might be possible to harmonize them, but it doesn't seem worth the effort right now.
And document that whole setup in the README.
@imbasimba This pull request now adds some documentation in the README with a bit more info about how the whole image-test subsystem works. |
I found the reason for a lot of the confusion and conflicting results and it is no wonder it was so difficult to diagnose and fix this issue! There were two separate problems interfering with my investigation. On top of the real bug there is a later commit slightly changing the zoom level WorldWideTelescope/wwt-webgl-engine@aeb5dc0 causing the tests to fail when I was working on master. This explains so much of my conflicting results! I'm glad I'm not crazy 😃 |
Great, thanks for the added documentation! |
Yeah, sorry that this is all such a mess :-/ From a certain standpoint, these image tests should be maintained in the wwt-webgl-engine repo to keep them "closer" to the engine changes. Side note: as mentioned in one of the commit messages in this PR, I noticed that the WebGL1 and WebGL2 renderers give fairly different results when opacities are used, as in the |
From a quick look it seems like the main difference is that opacity scales rgba in TileShader while in FitsShader opacity only affects the alpha channel. I can do a quick test before our next meeting to see if that's the only reason for the discrepancy. |
Yeah. I guess if opacity is scaling RGB and A, that would be incorrectly doubling its effect, right? Assuming that those colors aren't supposed to be premultiplied alpha. But overall, it's not something to prioritize, I'd say. |
Indeed the RGB is scaled twice in all shaders except the FitsShader. Basically this blender function makes the rgb * alpha unnecessary. Of course one could argue that there is no single correct answer when it comes to merging two images, but this seems like an honest mistake. |
Overview
In preparation for a new release, bump some dependency version requirements.
Checklist
N/A