Proposed updates for implementations of RenderState and StateRecord

archive development-discussion-jme2
1

In order to simply maintenance of both LWJGL and JOGL implementations, I propose that we make the following changes.  None of these changes would hinder other display systems implementations.  I've included a quick updated implementation of the JOGLCullState as an example.


  1. Make StateRecord classes generic by removing implementation specific details.  As an example, both the LWJGL and JOGL implementations of CullStateRecord currently use an int to hold the culled face.  By using the existing enumerated values in CullState.Face, the same operations can be performed without any display system specific constants.


public class CullStateRecord extends StateRecord {

    public boolean enabled = false;

    public Face face = null;

    public PolygonWind windOrder = null;

    @Override
    public void invalidate() {
        super.invalidate();

        enabled = false;
        face = null;
        windOrder = null;
    }

}



2. Make the RenderState classes generic by refactoring the code into two phases.  The first phase determines what should occur, and is generic JME code (see the apply() method).  The second phase is implementation specific but contains minimal logic (see applyCullEnabled(), applyCull(), and applyPolygonWind()).  Note the lack of any conditional or state logic within these methods.

public class JOGLCullState extends CullState {

    private static final long serialVersionUID = 1L;

    @Override
    public CullStateRecord createStateRecord() {
        return new CullStateRecord();
    }

    /**
     * @see com.jme.scene.state.RenderState#apply()
     */
    public void apply() {
        // ask for the current state record
        RenderContext<?> context = DisplaySystem.getDisplaySystem()
                .getCurrentContext();
        CullStateRecord record = (CullStateRecord) context
                .getStateRecord(RS_CULL);
        context.currentStates[RS_CULL] = this;

        // TODO Move default behavior to getCullFace()?
        Face useFace = getCullFace();
        if (useFace == null)
            useFace = Face.None;

        // TODO Move default behavior to getPolygonWind()?
        PolygonWind useWindOrder = getPolygonWind();
        if (useWindOrder == null)
            useWindOrder = PolygonWind.CounterClockWise;

        // Call enable if the state is enabled and a face is defined.
        boolean useEnable = isEnabled() && useFace != Face.None;
        if (!record.isValid() || record.enabled != useEnable) {
            applyCullEnabled(useEnable);
            record.enabled = useEnable;
        }

        // If culling is enabled, then push the face.
        if (useEnable) {
            if (!record.isValid() || record.face != useFace) {
                applyCull(useFace);
                record.face = useFace;
            }
        } else {
            // Default the winding order when culling is disabled.
            // FIXME Is this correct? this disabled state has an impact?
            useWindOrder = PolygonWind.CounterClockWise;
        }

        // Push the winding order independently since it can have an impact on
        // lighting.
        if (!record.isValid() || record.windOrder != useWindOrder) {
            applyPolygonWind(useWindOrder);
            record.windOrder = useWindOrder;
        }

        if (!record.isValid())
            record.validate();
    }

    /**
     * JOGL implementation.
     *
     * @param enable
     *            <code>true</true> to enable culling.
     */
    protected void applyCullEnabled(boolean enable) {
        final GL gl = GLU.getCurrentGL();
        if (enable) {
            gl.glEnable(GL.GL_CULL_FACE);
        } else {
            gl.glDisable(GL.GL_CULL_FACE);
        }
    }

    /**
     * JOGL implementation.
     *
     * @param face
     *            specifies which faces are candidates for culling.
     */
    protected void applyCull(Face face) {
        // Translate the enumerated value into an OpenGL constant.
        final int glFace;
        switch (face) {
        case Front:
            glFace = GL.GL_FRONT;
            break;
        case Back:
            glFace = GL.GL_BACK;
            break;
        case FrontAndBack:
            glFace = GL.GL_FRONT_AND_BACK;
            break;
        default:
            throw new IllegalStateException("Illegal Cull Face: " + face);
        }

        final GL gl = GLU.getCurrentGL();
        gl.glCullFace(glFace);
    }

    /**
     * JOGL implementation.
     *
     * @param windOrder
     *            specifies the orientation of front-facing polygons by the
     *            winding order.
     */
    protected void applyPolygonWind(PolygonWind windOrder) {
        // Translate the enumerated value into an OpenGL constant.
        final int glWindOrder;
        switch (windOrder) {
        case CounterClockWise:
            glWindOrder = GL.GL_CCW;
            break;
        case ClockWise:
            glWindOrder = GL.GL_CW;
            break;
        default:
            throw new IllegalStateException("Illegal polygon winding: "
                    + windOrder);
        }

        final GL gl = GLU.getCurrentGL();
        gl.glFrontFace(glWindOrder);
    }

}



3. Change the apply() method to accept a targeted renderer.  I haven't done this in the example, but this would make it possible to easily implement unit tests which cover all of the state's logic except for the implementation-specific code.  This change would also make the state's similar to the geometric objects, in that the implementation-specific code is left to an object provided as a parameter.
2

I'm open to this, as long as we also determined the impact on performance was nil or negligible.



The reason why records track by int currently is because I'd originally had the idea that we could ask OpenGL for state information and directly set it into the records without doing a bunch of switch/if statements.  That need has never really come up though, so I'm ok with switching to tracking by enums.



As for the comments about shifting null handling into the get methods, I'd rather not as that allows a discrete difference between something set and unset.  That's just my opinion though.

3

I was responding to the existing code base, so if other factors need to be considered I'd be happy to discuss them.



I don't expect the presented changes to have any performance impact, since the switch statements which converted enumerated types into OpenGL specific integer values already existed.  Most of the code changes really come down to reshuffling the existing code to separate the OpenGL implementation code.  I don't expect that the comparison of two enumerated types for equality will have an appreciable impact when compared to the equality of two integer values.



The null handling (default handling) can remain the same.  What's important is that the values are determined before the call to the OpenGL implementation methods, so that the StateRecord update can occur without shifting responsibility for state changes into the implementation methods.  I'm really thinking about these rendering methods as being stateless.