[Kde-games-devel] KGameRenderer Porting Experience

[Parker Coates]

On Fri, Jul 30, 2010 at 15:48, Stefan Majewsky wrote:
> On Wednesday 28 July 2010 20:23:37 Parker Coates wrote:
>> Do you think it's a good idea to add this class to the library to
>> avoid it being recreated multiple times?
>
> ACK.
> Please go ahead.

I'm out of town (yet again), but I'll be sure to do that when I get back.

>> 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.
>
> I'd rather insert a protected virtual function in KGR
> which is empty by default and gets called when a theme has been changed,
> like:
>
> virtual void themeChanged(const QString& oldTheme, const QString&
> newTheme) {}

Yeah, there are a few ways to do this to avoid making the public API
virtual. Protected virtuals like you mention or extra signals could
both work. I'm not really sure which I'd prefer, so I'm not going to
weigh in on that until I've had a chance to play around with it again.


On Sat, Jul 31, 2010 at 11:11, Stefan Majewsky wrote:
> On Thursday 29 July 2010 05:43:15 Parker Coates wrote:
>> There is already a themeChanged() signal in KGameRenderer. Initially I
>> was connecting it to a custom slot for to clear the cache, but then
>> ran into issues as some other slots attached to the same signal ended
>> up using the old cached properties before the clearCache slot got
>> called.
>
> Signals have, by definition, no
> reliable calling order for the attached slots. You could work around this
> limitation by connecting clearCache() with DirectConnection, and all other
> signals with a QueuedConnection, but that's really suboptimal.

Of course. I had played around with queued connections, but, as you
say, that only adds two levels of resolution and adds other
complications.

>> I guess one option for me would to add a themeChangedAndReady() signal
>> that only gets emitted once the theme has changed and the cache has
>> been cleared. Then I could use it for external connections instead of
>> themeChanged(). Not pretty, but not that bad.
>
> Actually, I consider this solution best. If
> you need more semantics in the calling order of the slots attached to
> themeChanged(), you want to define a new signal that implements the
> additional semantics.
>
> The other solution would be an themeAboutToBeChanged
> signal in the base class, which triggers clearCache() in Killbots. However,
> this solution has the problem that it is quite complex to determine when
> this signal needs to be emitted in the base class. KGameRenderer tries hard
> to abort a theme change if it runs into problems, and I do not want to emit
> the signal before it is clear that the theme will really
> change.

I just thought of a scenario where my themeChangedAndReady() signal
won't work. (Again, I'm sure this can be worked around. I'm just
throwing out potential issues.) In Killbots, the NumericDisplayItem
class is a KGameRenderedPixmapItem with a custom paint() method that
draws text on top of the background pixmap. The colour used to draw
the text is one of the cached properties I was talking about. So if
the cache isn't cleared by the time the KGameRendererClients are
updated, they'll repaint with the wrong text colour. This makes an
aboutToChange protected virtual or signal would be handy.

On Sat, Jul 31, 2010 at 13:14, Stefan Majewsky wrote:
> On Wednesday 28 July 2010 20:23:37 Parker Coates wrote:
>> 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.
>
> The
> name "KGameRenderedObject" is problematic; it should include "Item"
> somewhere to indicate that it's a QGraphicsItem.

I felt it parallels the QGraphicsItem/QGraphicsObject relationship
pretty well, but as I said in the original email, I could see where it
might also cause confusion.

>> 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.
>
> Revision 1157564 introduces a renderer pool that
> creates new QSvgRenderer instances as necessary to serve multiple threads.
> At the very least, this introduces no regressions in the tested games
> (KDiamond, Killbots, Kolf from my local work branch).

This is interesting. I'm assuming it raises memory usage a fair bit,
no? It's really too bad that QSvgRenderer doesn't provide any public
API that allows sharing the parsed, in memory representation of the
SVG (QSvgTinyDocument, I think it's called) as that would save on disk
access, parsing time and memory usage.

>> 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.
>
> What would be the usecase for
> a virtualized KGRC::setRenderSize() method? The only case in which code in
> libkdegames calls setRenderSize() is in
> KGameRenderedItemPrivate::adjustRenderSize, and because that's a QObject, we
> can add a renderSizeAdjusted() signal to it, should the need
> arise.

Nah. Having thought it over, I agree. That's mostly an explicitly
called method. There's probably no need to virtualise it.

Thanks for all the answers,

Parker