next »

[starscream]

Hi,

make my first steps through the code and found something at the registerXX-methods, that makes me pensively:

    public KeyboardDevice registerKeyboard(KeyboardDevice keyboard)
    {
        keyboard.registerListener( inputListener );
        keyboardDevices.add( keyboard );
       
        return( keyboard );
    }
   
[JavaDoc]

    public KeyboardDevice registerKeyboard(Canvas3D canvas)
    {
        KeyboardDevice keyboard = null;
       
        switch ( canvas.get3DPeer().getType() )
        {
            case JOGL:
                keyboard = new AWTKeyboard( canvas.get3DPeer().getComponent() );
                break;
               
            case LWJGL:
                keyboard = new LWJGLKeyboard( LWJGLKeyboard.InputMode.BUFFERED );
                break;
        }
       
        if (keyboard != null)
        {
            keyboard.registerListener( inputListener );
            keyboardDevices.add( keyboard );
        }
       
        return( keyboard );
    }
   


I think, the methods should be implemented at this way:

    public KeyboardDevice registerKeyboard(KeyboardDevice keyboard)
    {
        if (keyboard != null)
        {
            keyboard.registerListener( inputListener );
            keyboardDevices.add( keyboard );
        }
       
        return( keyboard );
    }
   
    /**
     * Registers a KeyboardDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     * @return the registered device itself
     */
    public KeyboardDevice registerKeyboard(Canvas3D canvas)
    {
        KeyboardDevice keyboard = null;
       
        switch ( canvas.get3DPeer().getType() )
        {
            case JOGL:
                keyboard = new AWTKeyboard( canvas.get3DPeer().getComponent() );
                break;
               
            case LWJGL:
                keyboard = new LWJGLKeyboard( LWJGLKeyboard.InputMode.BUFFERED );
                break;
        }
       
        return registerKeyboard( keyboard );
    }
   

Maybe i am just to fussy...

[Marvin Fröhlich]

Why that? The registerListener( KeyboardDevice ) method won't be called with a null argument. If it did, it is a logical error in the user's code. Please do correct me, but I don't see, why it is better this way.

Marvin

['n ddrylliog]

@starscream : if a null keyboard is passed through registerListener(), it should throw a NullPointerException, which is a very good thing indeed.

[Marvin Fröhlich]

@starscream : if a null keyboard is passed through registerListener(), it should throw a NullPointerException, which is a very good thing indeed.

It will.

Code:

keyboard.registerListener( inputListener );

This line does it.

Marvin

[starscream]

Sure, but two methods doing the same with different results?
And duplicated code?
I thought that it would be better, if there could only be one result (and a NullpointerException is a good hint to show trouble in the user-code) and only one point in the code, where the inputListener ist registered to the keyboard and this keyboard is added to the KeyboardDevices.
The code could look like this:

    public KeyboardDevice registerKeyboard(KeyboardDevice keyboard)
    {
        keyboard.registerListener( inputListener );
        keyboardDevices.add( keyboard );
        return( keyboard );
    }
   
    /**
     * Registers a KeyboardDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     * @return the registered device itself
     */
    public KeyboardDevice registerKeyboard(Canvas3D canvas)
    {
        KeyboardDevice keyboard = null;
       
        switch ( canvas.get3DPeer().getType() )
        {
            case JOGL:
                keyboard = new AWTKeyboard( canvas.get3DPeer().getComponent() );
                break;
               
            case LWJGL:
                keyboard = new LWJGLKeyboard( LWJGLKeyboard.InputMode.BUFFERED );
                break;
        }
       
        return registerKeyboard( keyboard );
    }



But as i told: maybe i am just to fussy ;-)

[Marvin Fröhlich]

The code could look like this:

Code:

    public KeyboardDevice registerKeyboard(KeyboardDevice keyboard)
    {
        keyboard.registerListener( inputListener );
        keyboardDevices.add( keyboard );
        return( keyboard );
    }
   
    /**
     * Registers a KeyboardDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     * @return the registered device itself
     */
    public KeyboardDevice registerKeyboard(Canvas3D canvas)
    {
        KeyboardDevice keyboard = null;
       
        switch ( canvas.get3DPeer().getType() )
        {
            case JOGL:
                keyboard = new AWTKeyboard( canvas.get3DPeer().getComponent() );
                break;
               
            case LWJGL:
                keyboard = new LWJGLKeyboard( LWJGLKeyboard.InputMode.BUFFERED );
                break;
        }
       
        return registerKeyboard( keyboard );
    }

Yes. You're right. I'll change it. Thanks.

Marvin


EDIT: Done.

[starscream]

i will take a look at my first xith-change 
Maybe i can help you later - if i understand the whole thing...

[starscream]

Checked out but found changes only at the registerKeyboard.
registerMouse seems to be the same:

    /**
     * Registers a MouseDevice on this object.
     *
     * @param mouse the MouseDevice to register
     * @return the registered device itself
     */
    public MouseDevice registerMouse(MouseDevice mouse)
    {
        mouse.registerListener( inputListener );
        mouseDevices.add( mouse );
       
        return( mouse );
    }
   
    /**
     * Registers a MouseDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     */
    public MouseDevice registerMouse(Canvas3D canvas)
    {
        MouseDevice mouse = null;
       
        switch ( canvas.get3DPeer().getType() )
        {
            case JOGL:
                mouse = new AWTMouse( canvas.get3DPeer().getComponent() );
                break;
               
            case LWJGL:
                mouse = new LWJGLMouse( LWJGLMouse.InputMode.POLLED );
                break;
        }
       
        return( registerMouse( mouse ) );
    }
   
    /**
     * Registers a Keyboard- and a MouseDevice on this object.
     *
     * @param keyboard the KeyboardDevice to register
     * @param mouse the MouseDevice to register
     */
    public void registerKeyboardAndMouse(KeyboardDevice keyboard, MouseDevice mouse)
    {
        registerKeyboard( keyboard );
        registerMouse( mouse );
    }
   

And directly after that ther is a method i do not understand too:


    /**
     * Registers a Keyboard- and a MouseDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     */
    public void registerKeyboardAndMouse(Canvas3D canvas)
    {
       if (!canvasRegisteredForInput.contains( canvas ))
        {
            registerKeyboard( canvas );
            registerMouse( canvas );
           
            canvasRegisteredForInput.add( canvas );
       }
    }


I do not understand the canvasRegisterForInput.
It is a private one which is just constructed in the Constructor and only used in this method. Do you realy need it anymore or is it a legacy one?
In my opinion the method should look like this:

    public void registerKeyboardAndMouse(Canvas3D canvas)
    {
            registerKeyboard( canvas );
            registerMouse( canvas );
    }

The reference to the canvasRegisterForInput should completely be removed.

And one more thing: thank you for Xith3D!

[Marvin Fröhlich]

Checked out but found changes only at the registerKeyboard.
registerMouse seems to be the same:

Stupid me! I forgot to change that for registerMouse. Thanks for the hint. It's changed now.

And directly after that ther is a method i do not understand too:

Code:

    /**
     * Registers a Keyboard- and a MouseDevice on this object.
     * It takes the value from canvas.getOGLLayer() and for
     * Jogl, AWT-Devices are created, for LWJGL LWJGL-Devices.
     *
     * @param canvas the Canvas3D to take the OGLLayer from
     */
    public void registerKeyboardAndMouse(Canvas3D canvas)
    {
    if (!canvasRegisteredForInput.contains( canvas ))
        {
            registerKeyboard( canvas );
            registerMouse( canvas );
           
            canvasRegisteredForInput.add( canvas );
    }
    }

I do not understand the canvasRegisterForInput.
It is a private one which is just constructed in the Constructor and only used in this method. Do you realy need it anymore or is it a legacy one?

Actually this is very new. It is to avoid to register a standard mouse and keyboard for a canvas twice. It doesn't make much sense to register a mouse or a keyboard twice to the same RenderLoop. This would always fire an input event twice (or more). So this check is a good one . And yes, it's just private and for internal use.

And one more thing: thank you for Xith3D!

You're welcome .

Marvin

['n ddrylliog]

And one more thing: thank you for Xith3D!

You're welcome, though kudos ought to go to Dave, Will, Yuri, JCD, Lilian, Croft, Kev, Arne, Flo, Bohdan,  and all others I forget here (forgive my Variable-ReliAbility-Memory (VRAM)  ).

Oh, and Marvin and me (of course)