API Review

Modules

audio

FIFE::SoundManager

prock - April 11th, 2013 - SoundManager isn't very robust. Requires some work like:

• sound groups - emitters can be added to groups to allow for modifying a group of sounds at once. Example of some logical groups are: Background sound effects, foreground effects, music. Groups could potentially be used for modifying the position of multiple emitters at once.
• emitters should be added to the map so positional sounds can be tracked and updated automatically as opposed to clients needed to track all that information. Attached to an instance perhaps?
• SoundClipManager reuses a lot of code from ImageManager for managing shared pointers. This code should be stripped and made into a template.
• Additional codec support (maybe not required as ogg seems to be working well)
• Information about the currently playing sound should be more readily available and accurate. For example when streaming a sound the current callback isn't always called exactly when the sound completes.
• Change the callback methodology into the Observer pattern as it is done throughout the rest of the engine.
• Better management of clips when playing the same clip multiple times at the same time OR when the maximum number of clips are playing at once. OpenAL has some limits here so the manager should determine which sounds it needs to cut short in order to play the requested clip.
• OpenAL errors needs to be handled more gracefully. Exceptions could be avoided in most cases I would imagine.
• Fade in/out effects could easily be added. Other effects need to be well thought out.

prock - April 11th, 2013 - In general the FIFE coding standard is followed there are a few variable alignment issues and maybe some member variable comment issues. Also some naming conventions issues.

controller

prock - April 12th, 2013 - The Engine does not create the gui manager. It is left up to the client, this however creates some memory boundry issues as the engine assumes ownership of the manager. I'm thinking instead of the hybrid gui manager we could add a mechanism that the client could add multiple gui managers here. As a suggestion the API could potentially be: guimanager = engine.addGuiManager("CEGUI") or guimanager = engine.addGuiManager("fifechan"), etc etc. Much like we do for the render backend. This however introduces an inflexibility as clients would be unable to define their own GUI manager (unless there is some C++ fu that can be done here).

prock - April 12th, 2013 - In EngineSettings there are a lot of cases where we use function names like setSDLRemoveFakeAlpha() or setGLUseFramebuffer(). These should actually be something like setSdlRemoveFakeAlpha() and setGlUseFrameBuffer() (I'm assuming here that framebuffer is actually 2 words?).

eventchannel

prock - April 12th, 2013 - As a general statement filenames are wrong. ec_ should be stripped. Also there are a lot of non standard member variable names. Variable alignments when defining should also be reviewed for pretty much every file in eventchannel. Member variables need to be documented properly.

prock - April 12th, 2013 - In EventManager we do some odd things with keyboard input. Take a look at processKeyEvent() and you'll see why. We had an issue with this come up recently. This is also why KeyFilter exists it seems.

base

prock - April 12th, 2013 - Odd but Event seems to be using SDL directly to get a timestamp. Should it not be using the FIFE time manager?

command

key

mouse

sdl

source

prock - April 12th, 2013 - EventSourceType is out of date as it has a reference to GUICHAN. I don't recall anyone ever using this but it should be fixed (as well as adding the other possible GUI engines.

ext

gui

prock - April 12th, 2013 - Any particular reason we pass the SDL event to the GUI managers? Couldn't we just pass the proper event type? This would reduce the coupling of SDL with the GUI managers. note see my comment on EventManager above regarding key events.

cegui

fifechan

hybrid

prock - April 12th, 2013 - Made a note about this in the Controller section. Could we get rid of this and allow clients to add multiple GUI managers to the engine itself?

librocket

loaders

audio

prock - April 12th, 2013

• Filenames should not have underscores
• Member variables need to be aligned
• Use of boost::scoped_ptr<> in SoundDecoderOgg should be reviewed

map

video

model

prock - April 12th, 2013

• Why does Model inherit FifeClass?
• member variable naming convention issues
• Created and adopted grids??? perhaps there is a better way?

metamodel

prock - April 12th, 2013

• more getId() issues here
• In FIFE::Object, setFilename() could perhaps be renamed as this is really just a means to store what file the object was loaded from. Could potentially extend it and add some meta data instead? like: setMetaData("key", "value");. Would be a quick and dirty way to add details about the object when needed.
• In TimeProvider::setMultiplier() we should not throw an exception as we can clamp it at 0. Warning in the log would suffice.
• TimeProvider - member variable name convention issues.
• CellGrid::getName() - there is a comment here asking if it's deprecated
• CellGrid should really be named ICellGrid.

structures

prock - April 12th, 2013

• Instance is very bloated. Not sure we'll be able to change this for FIFE 1.0 but we should try and think of ways to split this class up a bit.
• InstanceActionListener doesn't not conform to our naming conventions. It should be IInstanceActionListener. Same goes for InstanceChangeListener, and InstanceDeleteListener.
• Instance::getId() - string? integer? FifeId? this is confusing and not standard. We need to settle on seomthing here.
• Instance::getLocation() returns a copy of the location. Interesting note in the code comments: @note does not return const Location&, since swig wont be const correct
• Some names of functions could be reviewed here (not that they are bad). callOnActionFrame(),refresh(), etc.
• Instance should be re-organized. Variables and methods are kindof all over the place.
• CellChangeListener should be renamed to ICellChangeListener, same with CellDeleteListener, LayerChangeListener, MapChangeListener, etc etc
• FIFE::Map adopts camera... could change this to a factory?

pathfinder

savers

map

util

base

log

math

resource

structures

time

prock - April 11th, 2013 - Timer uses the callback pattern. Could probably change this to the Observer pattern quite easily. I don't see a real good reason not to use the observer pattern here.

utf8

vfs

dat

raw

zip

video

fonts

opengl

opengle

sdl

view

camera

helios - you can produce a nice bug with camera, get the location ref and change the coordinates, the camera will be on the old position until some other updates came up (or you have an attached instance or so)

renderers

helios - GenericRenderer is a good example... there were segfaults a few xears back... problem was that the RenderNode was not copied, instead the client own it, the good effect was that a client was able to manupluate the location without the need to send a new one