ScreenshotAppState refactoring

I have taken the time to refactor the ScreenshotAppState and made the system more extendable.

Key changes:

  • Split out the SceneProcessor from the AppState.
  • Separate the writeToFile aspect from the SceneProcessor, what happens to the screenshot after it is taken is now pluggable.
  • The file writing has support for custom naming schemes.

Code is here GitHub - kwando/jmonkeyengine at refactor_screenshots

ScreenshotAppState.java
[java]
/*

  • Copyright © 2009-2012 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 com.jme3.app.state;

import com.jme3.app.Application;
import com.jme3.app.state.ScreenshotProcessor.Screenshot;
import com.jme3.app.state.ScreenshotProcessor.ScreenshotHandler;
import com.jme3.input.InputManager;
import com.jme3.input.KeyInput;
import com.jme3.input.controls.ActionListener;
import com.jme3.input.controls.KeyTrigger;
import com.jme3.renderer.ViewPort;
import com.jme3.system.JmeSystem;
import java.io.File;
import java.util.List;
import java.util.logging.Logger;

public class ScreenshotAppState extends AbstractAppState implements ActionListener {

private static final Logger logger = Logger.getLogger(ScreenshotAppState.class.getName());

private DefaultNamingScheme namingScheme;
private ScreenshotHandler handler;
private final ScreenshotProcessor processor;

private static class DefaultNamingScheme implements WriteToFileStrategy.NamingScheme {

    private String shotName;
    private boolean numbered = true;
    private String filePath = null;

    /**
    * Using this constructor, the screenshot files will be written sequentially to the system
    * default storage folder.
     */
    private DefaultNamingScheme(){
    }

    /**
    * This constructor allows you to specify the output file path of the screenshot.
    * Include the seperator at the end of the path.
    * Use an emptry string to use the application folder. Use NULL to use the system
    * default storage folder.
    * @param filePath The screenshot file path to use. Include the seperator at the end of the path.
     */
    public DefaultNamingScheme(String filePath){
        this.filePath = filePath;
    }

    /**
    * This constructor allows you to specify the output file path of the screenshot.
    * Include the seperator at the end of the path.
    * Use an emptry string to use the application folder. Use NULL to use the system
    * default storage folder.
    * @param filePath The screenshot file path to use. Include the seperator at the end of the path.
     * @param shotName The name of the file to save the screeshot as.
     */
    public DefaultNamingScheme(String filePath, String shotName){
        this.filePath = filePath;
        this.shotName = shotName;
    }

    public File filenameForScreenshot(Screenshot screenshot) {
        File file;
        String filename;
        if (numbered) {
            filename = shotName + screenshot.getSequenceNumber();
        } else {
            filename = shotName;
        }
        if (filePath == null) {
            file = new File(JmeSystem.getStorageFolder() + File.separator + filename + ".png").getAbsoluteFile();
        } else {
            file = new File(filePath + filename + ".png").getAbsoluteFile();
        }
        return file;
    }
}

public ScreenshotAppState(DefaultNamingScheme scheme){
    super();
    this.namingScheme = scheme;
    processor = new ScreenshotProcessor(new WriteToFileStrategy(namingScheme));
}

/**
 * Using this constructor, the screenshot files will be written sequentially to the system
 * default storage folder.
 */
public ScreenshotAppState() {
    this(new DefaultNamingScheme());
}

/**
 * This constructor allows you to specify the output file path of the screenshot.
 * Include the seperator at the end of the path.
 * Use an emptry string to use the application folder. Use NULL to use the system
 * default storage folder.
 * @param filePath The screenshot file path to use. Include the seperator at the end of the path.
 */
public ScreenshotAppState(String filePath) {
    this(new DefaultNamingScheme(filePath));
}

/**
 * Set the file path to store the screenshot.
 * Include the seperator at the end of the path.
 * Use an emptry string to use the application folder. Use NULL to use the system
 * default storage folder.
 * @param filePath File path to use to store the screenshot. Include the seperator at the end of the path.
 */
public void setFilePath(String filePath) {
    namingScheme.filePath = filePath;
}

@Override
public void initialize(AppStateManager stateManager, Application app) {        
    setupControls(app);
    addProcessor(app);
    namingScheme.shotName = app.getClass().getSimpleName();

    super.initialize(stateManager, app);
}

private void addProcessor(Application app) {
    List<ViewPort> vps = app.getRenderManager().getPostViews();
    ViewPort last = vps.get(vps.size() - 1);
    last.addProcessor(processor);
}

private void setupControls(Application app) {
    InputManager inputManager = app.getInputManager();
    inputManager.addMapping("ScreenShot", new KeyTrigger(KeyInput.KEY_SYSRQ));
    inputManager.addListener(this, "ScreenShot");
}

public void onAction(String name, boolean value, float tpf) {
    if (value){
        processor.takeScreenshot();
    }
}

public void takeScreenshot() {
    processor.takeScreenshot();
}

}
[/java]

ScreenhotProcessor
[java]
/*

  • To change this license header, choose License Headers in Project Properties.
  • To change this template file, choose Tools | Templates
  • and open the template in the editor.
    */
    package com.jme3.app.state;

import com.jme3.post.SceneProcessor;
import com.jme3.renderer.Camera;
import com.jme3.renderer.RenderManager;
import com.jme3.renderer.Renderer;
import com.jme3.renderer.ViewPort;
import com.jme3.renderer.queue.RenderQueue;
import com.jme3.texture.FrameBuffer;
import com.jme3.util.BufferUtils;
import java.nio.ByteBuffer;

/**
*

  • @author kwando
    */
    public class ScreenshotProcessor implements SceneProcessor {

    public interface ScreenshotHandler {

     public void screenshotCaptured(Screenshot screenshot);
    

    }

    public static class Screenshot {

     private static int nextSequenceNumber = 1;
     private ByteBuffer buffer;
     private final int width;
     private final int height;
     private final int sequenceNumber;
    
     private Screenshot(ByteBuffer buffer, int width, int height) {
         this.buffer = buffer;
         this.width = width;
         this.height = height;
         sequenceNumber = nextSequenceNumber++;
     }
    
     protected ByteBuffer getBuffer() {
         return buffer;
     }
    
     public int getSequenceNumber() {
         return this.sequenceNumber;
     }
    
     public int getWidth() {
         return width;
     }
    
     public int getHeight() {
         return height;
     }
    

    }

    private boolean doCapture;
    private Renderer renderer;
    private RenderManager rm;
    private ByteBuffer outBuf;
    private int width, height;

    private ScreenshotHandler handler;

    public ScreenshotProcessor(ScreenshotHandler handler) {
    if (handler == null) {
    throw new IllegalArgumentException(“ScreenshotHandler cannot be null”);
    }
    this.handler = handler;
    }

    public void initialize(RenderManager renderManager, ViewPort vp) {
    renderer = rm.getRenderer();
    rm = renderManager;
    reshape(vp, vp.getCamera().getWidth(), vp.getCamera().getHeight());
    }

    public void reshape(ViewPort vp, int w, int h) {
    outBuf = BufferUtils.createByteBuffer(w * h * 4);
    width = w;
    height = h;
    }

    public boolean isInitialized() {
    return renderer != null;
    }

    public void preFrame(float tpf) {
    // Noop
    }

    public void postQueue(RenderQueue rq) {
    // Noop
    }

    public void postFrame(FrameBuffer out) {
    if (doCapture) {
    Screenshot screenshot = captureScreenshot(out);
    handler.screenshotCaptured(screenshot);
    doCapture = false;
    }
    }

    public void takeScreenshot() {
    this.doCapture = true;
    }

    public void cleanup() {
    // Noop
    }

    private Screenshot captureScreenshot(FrameBuffer out) {
    Camera curCamera = rm.getCurrentCamera();
    int viewX = (int) (curCamera.getViewPortLeft() * curCamera.getWidth());
    int viewY = (int) (curCamera.getViewPortBottom() * curCamera.getHeight());
    int viewWidth = (int) ((curCamera.getViewPortRight() - curCamera.getViewPortLeft()) * curCamera.getWidth());
    int viewHeight = (int) ((curCamera.getViewPortTop() - curCamera.getViewPortBottom()) * curCamera.getHeight());

     renderer.setViewPort(0, 0, width, height);
     renderer.readFrameBuffer(out, outBuf);
     renderer.setViewPort(viewX, viewY, viewWidth, viewHeight);
     return new Screenshot(outBuf, width, height);
    

    }
    }
    [/java]

WriteToFileStrategy.java
[java]
package com.jme3.app.state;

import java.io.File;
import com.jme3.app.state.ScreenshotProcessor.ScreenshotHandler;
import com.jme3.app.state.ScreenshotProcessor.Screenshot;
import com.jme3.system.JmeSystem;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
*

  • @author kwando
    */
    public class WriteToFileStrategy implements ScreenshotHandler {

    public interface NamingScheme {

     public File filenameForScreenshot(Screenshot screenshot);
    

    }

    private static final Logger logger = Logger.getLogger(ScreenshotAppState.class.getName());
    private NamingScheme namingScheme;

    public WriteToFileStrategy(NamingScheme namingScheme) {
    if (namingScheme == null) {
    throw new IllegalArgumentException(“Namingscheme cannot be nulll”);
    }
    this.namingScheme = namingScheme;
    }

    public void screenshotCaptured(Screenshot screenshot) {
    writeScreenshotToFile(screenshot, namingScheme.filenameForScreenshot(screenshot));
    }

    private void writeScreenshotToFile(Screenshot screenshot, File file) {
    writeFile(file, screenshot.getBuffer(), screenshot.getWidth(), screenshot.getHeight());
    }

    private void writeFile(File file, ByteBuffer outBuf, int width, int height) {
    OutputStream outStream = null;
    try {
    outStream = new FileOutputStream(file);
    JmeSystem.writeImageFile(outStream, “png”, outBuf, width, height);
    } catch (IOException ex) {
    logger.log(Level.SEVERE, “Error while saving screenshot”, ex);
    } finally {
    if (outStream != null) {
    try {
    outStream.close();
    } catch (IOException ex) {
    logger.log(Level.SEVERE, “Error while saving screenshot”, ex);
    }
    }
    }
    }
    }
    [/java]

I need some help with how I can share the ByteBuffer from the ScreenshotProcessor.Screenshot with user implemented handlers.

4 Likes

I guess nobody really has an opinion on this. Is everything working as before? Then I guess we can add this…

I don’t have strong opinions but I was hoping someone else would comment first. :slight_smile:

What is the reasoning for breaking it out into multiple classes? Do you see reuse of these different parts or was it just because “the class was getting too big”? If you see reusing the different parts that you’ve broken out, can you describe how they might be used? Specifically I’m thinking of the split out processor.

The naming scheme thing seems like it might be a little bit of overkill since one can already override how the file is written, but I don’t know. Signs of how this is confusing things is that you have a default name handler defined in the main class even though it’s using the interface defined in the strategy class… would make more sense for the default handling to be in the strategy class in the first place… and then it starts to feel like there doesn’t need to be a separate class at all.

It’s funny, it seems like in this refactoring that the one thing it doesn’t is the thing I’ve always wanted: to change the key.

Uh, I missed the replies to this thread =)

I will try explain the reasoning behind this refactor.

First, splitting out the scene processor is mostly because it is very confusing having a class implement SceneProcessor and AppState at the same time. Those interfaces share some methods and at least I had to think a second time when I traced that through that code. Also the SceneProcessor is the only part I plan to use and the sole reason I did all of this =P

The ScreenshotAppState is still ugly because it needs to be backwards compatible with 3.0, though I reverted some of the changes that was made to the AppState that is not in stable yet. Those changes just cluttered up the class with methods for customizing the filename, which I do not believe is the core responsibility of that class.

The DefaultNamingScheme (maybe named as LegacyNamingScheme?) is only implemented because I have to be compatible with 3.0. I think users should define their own naming scheme if they need something more fancy. I can merge the AppState and the WriteToFIleStrategy class, they do not really need be separate… but I like to have many objects instead of largish objects with to many responsibles.

That being said, I won’t be that sad if we decide not to merge this… but then I think we should remove the new file naming methods that is not in stable yet.

@kwando said: Uh, I missed the replies to this thread =)

I will try explain the reasoning behind this refactor.

First, splitting out the scene processor is mostly because it is very confusing having a class implement SceneProcessor and AppState at the same time. Those interfaces share some methods and at least I had to think a second time when I traced that through that code. Also the SceneProcessor is the only part I plan to use and the sole reason I did all of this =P

The ScreenshotAppState is still ugly because it needs to be backwards compatible with 3.0, though I reverted some of the changes that was made to the AppState that is not in stable yet. Those changes just cluttered up the class with methods for customizing the filename, which I do not believe is the core responsibility of that class.

The DefaultNamingScheme (maybe named as LegacyNamingScheme?) is only implemented because I have to be compatible with 3.0. I think users should define their own naming scheme if they need something more fancy. I can merge the AppState and the WriteToFIleStrategy class, they do not really need be separate… but I like to have many objects instead of largish objects with to many responsibles.

That being said, I won’t be that sad if we decide not to merge this… but then I think we should remove the new file naming methods that is not in stable yet.

Yeah, maybe. Before it becomes yet another backwards compatibility issue.

I don’t have a problem with having a write strategy… it just seemed a little superfluous to also have a pluggable file name strategy since write strategy a) could do that, and b) may have no need for a file name at all depending on how it’s storing it. I do like the idea of a write strategy because then you can also shunt it to another thread if desired.

so what do we do of this PR?

@pspeed I don't have a problem with having a write strategy... it just seemed a little superfluous to also have a pluggable file name strategy since write strategy a) could do that, and b) may have no need for a file name at all depending on how it's storing it. I do like the idea of a write strategy because then you can also shunt it to another thread if desired.

a) That would mean extending the WriteToFileStrategy just to change the filename.
b) If you don’t wanna write the screenshot to a file you should implement a new handler. I wish we could expose a constructor/method which injects a custom handler in the ScreenshotAppState class, but the public API of that class assumes the screenshot is getting saved to a file so it has to depend on WriteToFileStrategy instead of the ScreenshotHandler interface. This refactor would be more useful if I can change the public API slightly to not expose those file naming methods on ScreenshotAppState.

Another thing I’m thinking about is, that ByteBuffer the frame buffer is copied into is shared between Screenshot instances (unless no reshaping has been done). Would it be terrible idea to allocate a new ByteBuffer for every screenshot instead? That way one could safely process a captured screenshot on another thread (as I also intend to do).

@kwando said: a) That would mean extending the WriteToFileStrategy just to change the filename. b) If you don't wanna write the screenshot to a file you should implement a new handler. I wish we could expose a constructor/method which injects a custom handler in the ScreenshotAppState class, but the public API of that class assumes the screenshot is getting saved to a file so it has to depend on WriteToFileStrategy instead of the ScreenshotHandler interface. This refactor would be more useful if I can change the public API slightly to not expose those file naming methods on ScreenshotAppState.

Another thing I’m thinking about is, that ByteBuffer the frame buffer is copied into is shared between Screenshot instances (unless no reshaping has been done). Would it be terrible idea to allocate a new ByteBuffer for every screenshot instead? That way one could safely process a captured screenshot on another thread (as I also intend to do).

That doesn’t seem like the greatest idea. I no longer remember off the top of my head the chain of buffers but would want to avoid adding anymore memory overhead to this process. In my own testing, simply shunting the resulting image over to another thread is enough to prevent lag. It’s the file writing that takes a long time.

Personally, I think SceenshotAppState should be the backwards compatibility point by tying together the key, the processor, and file writing. I’m fine with a user who wants to change the way the file name is generated having to extend the write strategy because I think that’s pretty rare. The file write strategy could even take a custom format string and publish in the javadocs what objects it will pass that string or something to appease the most common cases.

Anyone who wants more complicated behavior than that can roll their own app state using the parts you’ve provided… which should be easy by then because the app state is just doing wiring by then. ie: configures the scene processor with the write strategy, configures some key binding… done.

@pspeed said: That doesn't seem like the greatest idea. I no longer remember off the top of my head the chain of buffers but would want to avoid adding anymore memory overhead to this process. In my own testing, simply shunting the resulting image over to another thread is enough to prevent lag. It's the file writing that takes a long time.
I can confirm it is the file writing / png conversion that is taking the time. The main problem with the shared ByteBuffer is that it is mutable and if your async handler is not done with the processing before next screenshot is taken that buffer will be overwritten... so either allocate a new buffer for each screenshot, or we need some mechanism for handlers to send "I'm done with this screenshot, it is safe for you reuse the ByteBuffer" message back to the screenshot processor.
@pspeed I'm fine with a user who wants to change the way the file name is generated having to extend the write strategy because I think that's pretty rare.
It was not rare enough since someone felt the need to add a bunch of new methods for generating filenames...
@pspeed Anyone who wants more complicated behavior than that can roll their own app state using the parts you've provided... which should be easy by then because the app state is just doing wiring by then. ie: configures the scene processor with the write strategy, configures some key binding... done.
Sure, that was the point of this refactor.
@kwando said: I can confirm it is the file writing / png conversion that is taking the time. The main problem with the shared ByteBuffer is that it is mutable and if your async handler is not done with the processing before next screenshot is taken that buffer will be overwritten... so either allocate a new buffer for each screenshot, or we need some mechanism for handlers to send "I'm done with this screenshot, it is safe for you reuse the ByteBuffer" message back to the screenshot processor.

I thought it went:
ByteBuffer
to BufferedImage
to ImageIO

Then I remembered that was in my version because I needed to clean up the transparency… since screen shots were being stored with alpha and it looked weird.

Anyway, copying the buffer is up to the write strategy. If it’s sending it off to another thread then it can copy it first. The screen shot processor should not be doing that.

@kwando said: It was not rare enough since someone felt the need to add a bunch of new methods for generating filenames...

Yes, and that basic functionality can be provided by the file writing strategy with a convenient method to override for something else. If we don’t stop somewhere, we end up with strategies calling strategies to call strategies. “What if my string template wants a different integer?!?! We should have a SequenceNumberStrategy!!” It gets pretty crazy if we don’t draw the line.

Provide a useful write to file strategy that supports existing functionality and let others subclass + override if they want something different. ScreenShotAppState then becomes a binding of the file write strategy, the scene processor, and some key. All of it’s existing methods can pass through to one of those things.

Bump as the pull request is still open and neither merged not closed.

What will we do with this?

I think the consensus of our discussions is that this pull request should be changed in order to be less invasive.

It should not be merged in its current condition.

That’s my take as well… since I seemed to be leveling most of the ‘please change this’. :slight_smile: