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

Send Screenshot downstream for testing fix #2015

Merged
merged 15 commits into from Nov 26, 2019
Merged

Conversation

@Haydelj
Copy link
Contributor

Haydelj commented Oct 28, 2019

Fixes view scenes output to contain an up to date frame

@dcwhite dcwhite requested review from dcwhite and jessdtate Oct 29, 2019
@dcwhite dcwhite self-assigned this Oct 29, 2019
@dcwhite dcwhite added the Graphics label Oct 29, 2019
@dcwhite dcwhite added this to New Features in Renderer/Visualization Oct 29, 2019
@dcwhite dcwhite added this to the 5.0-beta.X [Internal] milestone Oct 29, 2019
Copy link
Member

dcwhite left a comment

Clean up these memory/scope things and I'll test this out

src/Modules/Render/ViewScene.cc Outdated Show resolved Hide resolved
src/Modules/Render/ViewScene.cc Outdated Show resolved Hide resolved
src/Modules/Render/ViewScene.cc Outdated Show resolved Hide resolved
src/Interface/Modules/Render/ViewScene.h Outdated Show resolved Hide resolved
Haydelj added 2 commits Oct 29, 2019
@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Oct 29, 2019

This currently breaks widgets. @RubioJr9 and I will have to take a look at why.

Haydelj
@jessdtate

This comment has been minimized.

Copy link
Contributor

jessdtate commented Oct 31, 2019

Breaks widgets, probably because it doesn't ever finish executing.

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 1, 2019

Seems to break everything...

dcwhite added 2 commits Nov 4, 2019
@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 6, 2019

@dcwhite Todo: add a lock log to see what's blocking what

@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Nov 6, 2019

On my machine, this fix works fine with and without widgets. I'll try building it on Linux and see if that's any different.

@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Nov 6, 2019

I was able to replicate the issue and the new core dump feature revealed it is being caused by a problem in the shader promise system. The issue is that the promise is fulfilled but never removed.

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 6, 2019

Cool!

@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Nov 14, 2019

@dcwhite @jessdtate I think I fixed the issue will y'all test it on your machines to varify

@jessdtate

This comment has been minimized.

Copy link
Contributor

jessdtate commented Nov 14, 2019

The merge conflicts make it hard to test

@jessdtate

This comment has been minimized.

Copy link
Contributor

jessdtate commented Nov 14, 2019

seems to behaving like you described before, ie, it will finish executing, but only if the render window is open. This is improvement. Could we add a check for the window being open so it doesn't get stuck?

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 14, 2019

@Haydelj That's not a platform-neutral way to sleep--try https://en.cppreference.com/w/cpp/thread/sleep_for

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 14, 2019

@Haydelj And do please fix the merge conflict

Haydelj added 2 commits Nov 15, 2019
@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Nov 15, 2019

@jessdtate @dcwhite I fixed the things y'all requested I fix. Now execution will continue even if the window is closed

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 20, 2019

I get this bad crash with multiple re-executes:
SCIRun_test(71368,0x70000efcd000) malloc: Double free of object 0x7f7fc75e29f0
Let me get a more consistent replication script

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 20, 2019

Haven't been able to replicate the crash.

What I am seeing though, and most regression networks can be used to test this, is that the Matrix output from ViewScene doesn't work until the VS window is open. Is that expected at this point? It's true there is no longer any hang.

@Haydelj

This comment has been minimized.

Copy link
Contributor Author

Haydelj commented Nov 20, 2019

@dcwhite yes due to the limitations of using QOpenGlWidget as our render target that is expected behavior. Qt won't let us render when the window is hidden. We could render to texture directly to get around this but I believe that would suffer from the same bug that the selection buffer rendering suffers from. It also wouldn't solve the problem that context initialization doesn't happen until the window is shown.

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 20, 2019

Right, ok. Let me test some other regression network cases

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 21, 2019

Regression tests are passing (well enough at least). While I'm no fan of arbitrarily throwing sleep calls around to solve deadlock problems, this does seem to work for now. Next we need to figure out how to get the screenshot data out in a guaranteed way downstream to enable some actual image-based regression testing.

@dcwhite

This comment has been minimized.

Copy link
Member

dcwhite commented Nov 21, 2019

Actually, on Windows, this might have been a first: all regression networks passed! Before, there were always random crashes coming from ViewScene code. So this is a great improvement in functionality, assuming that double-delete crash was just a fluke...(it was on Mac btw).

@dcwhite dcwhite merged commit 431d76f into SCIInstitute:master Nov 26, 2019
6 of 7 checks passed
6 of 7 checks passed
label label
Details
linux-build (ubuntu-16.04)
Details
linux-build (ubuntu-18.04)
Details
mac-build
Details
windows-build
Details
CodeFactor No issues found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Renderer/Visualization automation moved this from New Features to Done Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.