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

solution and testcase for issue #1421 (ScreenshotAppState never cleans up) #1424

Merged
merged 3 commits into from Nov 23, 2020
Merged

solution and testcase for issue #1421 (ScreenshotAppState never cleans up) #1424

merged 3 commits into from Nov 23, 2020

Conversation

stephengold
Copy link
Member

No description provided.

@stephengold stephengold added this to the Future Release milestone Nov 15, 2020
@MeFisto94
Copy link
Member

MeFisto94 commented Nov 16, 2020

I couldn't connect how a SceneProcessor could cleanup the AbstractAppState initally, but apparently there is a name clash between SceneProcessor#cleanup and AbstractAppState#cleanup, where from AAS we call SP#cleanup, which in turn invokes on our AppState.

Your workaround will work, super.cleanup is no problem for the current code (because it only sets a boolean), but there might be a more clever way.
I am sure @pspeed42 has had this case a few times already and knows something clever?

What I could imagine is that making the Processor an internal class (could even be anonymous, I guess) would make this code more clean and avoid such a hack, then.
Essentially those are two classes anyway, the appstate only does the lifecycle management of the Processor.

@pspeed42
Copy link
Contributor

Since I was tagged: Yeah, my own personal idiomatic approach is to avoid excessive multiclassing just for (fake) convenience. So that app state has to be an app state because that's what it is. It should not also be a scene processor... because it's an app state. The issue described is one of a half-dozen pitfalls to the 'quick and dirty' approach uses in this app state... but probably too long to go into here.

@stephengold stephengold changed the title solution and testcase for issue #1421 solution and testcase for issue #1421 (ScreenshotAppState never cleans up) Nov 18, 2020
@stephengold
Copy link
Member Author

If there's no further discussion, I'd like to integrate this PR today.

@stephengold stephengold merged commit a3c3678 into jMonkeyEngine:master Nov 23, 2020
@stephengold stephengold deleted the sgold-issue-1421 branch November 23, 2020 17:07
@stephengold stephengold modified the milestones: Future Release, v3.4.0 Mar 13, 2021
@stephengold stephengold linked an issue Mar 16, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempted to add mapping "ScreenShot" twice to trigger
3 participants