[Kde-games-devel] KGameRenderer Porting Experience

[Parker Coates]

Hello Stefan and all,

So last week I ported Killbots to the new KGameRenderer framework and
just committed the changes to trunk. (See commits 1156142 through
1156149.) The following are some topics that came up during the
process that I'd like to hear some thoughts on. Sorry for the length.


=== KGameRenderedItem vs KGameRenderedPixmapItem ===

As I've expressed in some previous mails, I'm a bit concerned about
the complexity of KGameRenderedItem. During my port I eventually
realised that what I was wanting was just a QGraphicsPixmapItem that
would automatically update via KGameRenderer. I can see the benefit of
inheriting from QGraphicsObject in some scenarios, but in many others
I just don't think it's worth the complication it adds. In the end,
the class I want is nothing more than:

class KGameRenderedPixmapItem : public QGraphicsPixmapItem, public
KGameRendererClient
{
public:
   KGameRenderedPixmapItem( KGameRenderer * renderer, const QString &
spriteKey, QGraphicsItem * parent = 0 )
     : QGraphicsPixmapItem( parent ),
       KGameRendererClient( renderer, spriteKey )
{
}

protected:
   virtual void receivePixmap( const QPixmap & pixmap )
   {
       setPixmap( pixmap );
   }
};

Do you think it's a good idea to add this class to the library to
avoid it being recreated multiple times? The name is a bit verbose,
though. I was thinking that maybe we could rename the current
KGameRenderedItem to KGameRenderedObject and rename this new class
KGameRenderedItem, but I'm not sure whether that makes the distinction
between the classes clearer or more confusing.


=== Thread Safety ===

While reading through the code, I noticed that KGR uses a single
QSvgRenderer instance from multiple threads without using a mutex. QSR
is reentrant, but not thread-safe so it needs to be protected by a
mutex or similar locking device. The current code seems to work fine,
but I doubt we want to trust that it will continue to.


=== Virtualising Methods ===

During my port I ran into issues a couple times where I wanted to
reimplement an method, only to find that it wasn't virtual. In a
couple cases I easily found a workaround by restructuring the code
slightly, but in one particular case, I'm stuck.

Killbots::Renderer (a KGR subclass) caches a few custom theme
properties. Obviously, these cached values need to be cleared when a
new theme is loaded, so I added code to do just that in setTheme().
But setTheme() isn't virtual, so if it is called with a KGameRenderer
pointer, the original implementation gets called and my cache isn't
cleared. At first I didn't think this was a big deal as I could always
just use a Killbots::Renderer pointer in Killbots code (there's only
one instance anyway), but KGR actually makes calls to setTheme
internally under certain situations such as when loading an invalid
theme. In such cases, my cache won't get cleared.

Now I'm certainly not saying that we should make everything virtual
just in case someone someday might need to customise it, but there are
probably some methods that are more likely to need custom code.
Personally, I think KGameRenderer::setTheme() and
KGameRendererClient::setRenderSize() are likely candidates, but there
may be others.


=== QPixmap Warnings ===

Games using KGR in a K_GLOBAL_STATIC (ex: KDiamond, Killbots)
currently generate a lot of "~QX11PixmapData(): QPixmap objects must
be destroyed before the QApplication object, otherwise the native
pixmap object will be leaked." warnings at shutdown. Do we care? If we
do, should be come up with a standard pattern for dealing with the
issue?


=== General Awesomeness ===

I just wanted to conclude by saying that despite all the bitching and
moaning above, I think this project has turned out really well. Even
if we totally ignore the performance benefits, I'm sure KGR and
friends will make KDE games much easier to write and maintain in the
future. Excellent work, Stefan.


Parker