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

* add test case for issue #1421 (ScreenshotAppState never cleans up)

* solution for issue #1421 (ScreenshotAppState never cleans up)

* ScreenshotAppState: more commentary regarding the dodgy design

jme3-core/src/main/java/com/jme3/app/state/ScreenshotAppState.java

@@ -68,6 +68,14 @@ public class ScreenshotAppState extends AbstractAppState implements ActionListen
     private long shotIndex = 0;
     private int width, height;
     private AppProfiler prof;
+    /**
+     * InputManager to which the ActionListener and the mapping are added
+     */
+    private InputManager inputManager;
+    /**
+     * ViewPort to which the SceneProcessor is attached
+     */
+    private ViewPort last;
 
     /**
      * Using this constructor, the screenshot files will be written sequentially to the system
@@ -170,13 +178,13 @@ public class ScreenshotAppState extends AbstractAppState implements ActionListen
 
     @Override
     public void initialize(AppStateManager stateManager, Application app) {
-        if (!super.isInitialized()){
-            InputManager inputManager = app.getInputManager();
+        if (!super.isInitialized()) {
+            inputManager = app.getInputManager();
             inputManager.addMapping("ScreenShot", new KeyTrigger(KeyInput.KEY_SYSRQ));
             inputManager.addListener(this, "ScreenShot");
 
             List<ViewPort> vps = app.getRenderManager().getPostViews();
-            ViewPort last = vps.get(vps.size()-1);
+            last = vps.get(vps.size() - 1);
             last.addProcessor(this);
 
             if (shotName == null) {
@@ -187,6 +195,42 @@ public class ScreenshotAppState extends AbstractAppState implements ActionListen
         super.initialize(stateManager, app);
     }
 
+    /**
+     * Clean up this AppState during the first update after it gets detached.
+     * <p>
+     * Because each ScreenshotAppState is also a SceneProcessor (in addition to
+     * being an AppState) this method is also invoked when the SceneProcessor
+     * get removed from its ViewPort, leading to an indirect recursion:
+     * <ol><li>AppStateManager invokes ScreenshotAppState.cleanup()</li>
+     * <li>cleanup() invokes ViewPort.removeProcessor()</li>
+     * <li>removeProcessor() invokes ScreenshotAppState.cleanup()</li>
+     * <li>... and so on.</li>
+     * </ol>
+     * <p>
+     * In order to break this recursion, this method only removes the
+     * SceneProcessor if it has not previously been removed.
+     * <p>
+     * A better design would have the AppState and SceneProcessor be 2 distinct
+     * objects, but doing so now might break applications that rely on them
+     * being a single object.
+     */
+    @Override
+    public void cleanup() {
+        if (inputManager != null) {
+            inputManager.deleteMapping("ScreenShot");
+            inputManager.removeListener(this);
+            inputManager = null;
+        }
+
+        ViewPort viewPort = last;
+        if (viewPort != null) {
+            last = null;
+            viewPort.removeProcessor(this); // XXX indirect recursion!
+        }
+
+        super.cleanup();
+    }
+
     @Override
     public void onAction(String name, boolean value, float tpf) {
         if (value){
@@ -219,10 +263,12 @@ public class ScreenshotAppState extends AbstractAppState implements ActionListen
 
     @Override
     public void preFrame(float tpf) {
+        // do nothing
     }
 
     @Override
     public void postQueue(RenderQueue rq) {
+        // do nothing
     }
 
     @Override

jme3-examples/src/main/java/jme3test/app/state/TestIssue1421.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2020 jMonkeyEngine
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ *   notice, this list of conditions and the following disclaimer in the
+ *   documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of 'jMonkeyEngine' nor the names of its contributors
+ *   may be used to endorse or promote products derived from this software
+ *   without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package jme3test.app.state;
+
+import com.jme3.app.SimpleApplication;
+import com.jme3.app.state.ScreenshotAppState;
+import com.jme3.post.SceneProcessor;
+import com.jme3.renderer.ViewPort;
+import java.util.List;
+
+/**
+ * Test case for JME issue #1421: ScreenshotAppState never cleans up.
+ * <p>
+ * If successful, the application will complete without thowing an Exception.
+ *
+ * @author Stephen Gold
+ */
+public class TestIssue1421 extends SimpleApplication {
+
+    private int updateCount = 0;
+    private ScreenshotAppState screenshotAppState;
+
+    public static void main(String[] args) {
+        TestIssue1421 app = new TestIssue1421();
+        app.start();
+    }
+
+    @Override
+    public void simpleInitApp() {
+        // enable screenshots
+        screenshotAppState = new ScreenshotAppState("./", "screen_shot");
+        boolean success = stateManager.attach(screenshotAppState);
+        assert success;
+    }
+
+    @Override
+    public void simpleUpdate(float tpf) {
+        ++updateCount;
+        if (updateCount == 10) { // after attached
+            // Confirm that the SceneProcessor is attached.
+            List<ViewPort> vps = renderManager.getPostViews();
+            assert vps.size() == 1 : vps.size();
+            ViewPort lastViewPort = vps.get(0);
+            List<SceneProcessor> processorList = lastViewPort.getProcessors();
+            int numProcessors = processorList.size();
+            assert numProcessors == 1 : numProcessors;
+
+            // Confirm that KEY_SYSRQ is mapped.
+            assert inputManager.hasMapping("ScreenShot");
+
+            // disable screenshots
+            boolean success = stateManager.detach(screenshotAppState);
+            assert success;
+
+        } else if (updateCount == 20) { // after detached
+            // Check whether the SceneProcessor is still attached.
+            List<ViewPort> vps = renderManager.getPostViews();
+            ViewPort lastViewPort = vps.get(0);
+            List<SceneProcessor> processorList = lastViewPort.getProcessors();
+            int numProcessors = processorList.size();
+            if (numProcessors != 0) {
+                throw new RuntimeException("SceneProcessor is still attached.");
+            }
+
+            // Check whether KEY_SYSRQ is still mapped.
+            if (inputManager.hasMapping("ScreenShot")) {
+                throw new RuntimeException("KEY_SYSRQ is still mapped.");
+            }
+            stop();
+        }
+    }
+}