[Kde-games-devel] Review Request: Port KNetWalk to use QGraphicsView and KGameRenderer

Parker Coates parker.coates at kdemail.net
Mon Sep 6 05:01:22 CEST 2010 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5238/#review7417
-----------------------------------------------------------


So first off, I'd just like to say that I've been meaning to port KNetWalk for a while now, but never really found the time. It's probably my favourite game in the entire module. Thanks for giving it a shot. For the most part things look good.

Normally, I would say that this patch is too big. Porting from widgets to a canvas and switching the rendering framework in a single diff means that there's a lot of (not always related) things going on. But due to the coupling between the two frameworks, splitting it up into two diffs would probably mean extra work and wouldn't be worthwhile. In the future, please try to keep unrelated changes (like game pausing) in their own patches. It's much easier to review five ten-line patches than one thirty-line patch.


/trunk/KDE/kdegames/knetwalk/src/cell.h
<http://svn.reviewboard.kde.org/r/5238/#comment7626>

    Embedding a second QGraphicsRectItem inside a QGraphicsRectItem seems a bit much. Did you consider just mixing the colours in a single QGraphicsRectItem?



/trunk/KDE/kdegames/knetwalk/src/cell.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7624>

    Not a big deal, but a nicer way to do this would be to have fillNameHash return a QHash<int,QByteArray> so that you can just do "const QHash<int, QByteArray> Cell::s_directionNames = fillNameHash();" That way you can make it const and remove the empty() check every time you create a cell.



/trunk/KDE/kdegames/knetwalk/src/cell.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7625>

    In Qt code we tend to prefer isEmpty() to empty(). Again, not a big deal.



/trunk/KDE/kdegames/knetwalk/src/cell.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7627>

    This code really should be ported to the new QAnimation stuff in the future. Not that that has anything to do with this patch. :)



/trunk/KDE/kdegames/knetwalk/src/cell.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7616>

    You still get that tiny graphic artefact if add a "m_cablesItem->setTransformationMode(Qt::SmoothTransformation);" here although it does seem to be a bit less noticeable.
    
    Speaking of which, enabling smooth transformations makes the rotation animations look *much* nicer, but I'm not sure how much more computationally expensive it is. It would take some profiling (that I don't have time to do to ;)) to find out.



/trunk/KDE/kdegames/knetwalk/src/fielditem.h
<http://svn.reviewboard.kde.org/r/5238/#comment7622>

    Unused include?



/trunk/KDE/kdegames/knetwalk/src/fielditem.h
<http://svn.reviewboard.kde.org/r/5238/#comment7621>

    paint() is public in QGraphicsItem and the eventHandlers are protected. Why make them private here? Especially considering that paint() has to be called publicly to work (but thanks to the miracle of virtual functions, it still does). ;)



/trunk/KDE/kdegames/knetwalk/src/fielditem.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7623>

    Instead of implementing an empty paint() method, you should set the ItemHasNoContents flag as it's more efficient.



/trunk/KDE/kdegames/knetwalk/src/gamewidget.h
<http://svn.reviewboard.kde.org/r/5238/#comment7617>

    I would encourage you to move these two classes to separate files with appropriate names. When I started looking through the code, I immediately thought "Hey, where's the scene? And I thought he said he got rid of the widgets?" :)



/trunk/KDE/kdegames/knetwalk/src/gamewidget.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7629>

    For efficiency reasons, you may want to consider getting rid of m_bgOverlay and combining the "overlay" and "background" into a single pixmap in here. The reason is that QGraphicsView is able to cache the background.
    
    By keeping the overlay as a separate item, that means that the overlay has to be (at least partially) repainted every time one of the items over top of it changes. Since it's basically a part of the background, you might as well stick them together.



/trunk/KDE/kdegames/knetwalk/src/gamewidget.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7630>

    You might want to play with adding some of the QGV optimisations here. See setCacheMode(), setViewportUpdateMode(), and setOptimizationFlags();



/trunk/KDE/kdegames/knetwalk/src/mainwindow.h
<http://svn.reviewboard.kde.org/r/5238/#comment7618>

    Where did this stuff go?



/trunk/KDE/kdegames/knetwalk/src/mainwindow.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7619>

    ?



/trunk/KDE/kdegames/knetwalk/src/mainwindow.cpp
<http://svn.reviewboard.kde.org/r/5238/#comment7620>

    Why get rid of the Renderer singleton? You now have a few classes holding and passing around pointers to a KGameRenderer object, which I don't really love. Getting rid of the singleton is fine if you can cleanly and conveniently keep everyone informed without it, but I'm really not sure that this is an improvement.


Oh, and I had the same issue as Stefan. The initial view was solid white and stayed that way after resizing the window. Only after changing the theme could I get some visuals. Now that I've got them, they seem to be sticking around on subsequent runs, so you might want to look at how you handle the case of an empty cache?

Sorry if that seems like an overwhelming number of complaints. Please don't let it scare you away. :) Really I'm quite pleased with the patch overall.

- Parker


On 2010-09-02 22:36:16, Brian Croom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5238/
> -----------------------------------------------------------
> 
> (Updated 2010-09-02 22:36:16)
> 
> 
> Review request for KDE Games and Stefan Majewsky.
> 
> 
> Summary
> -------
> 
> As KNetWalk's use of QWidget as its graphics stack was marked in red on the porting status wiki page, and because I wanted to see if I had acquired a good enough working understanding of the QGraphicsView framework to be able to accomplish it, I decided to attempt a full port of this game. I referred to the KMines source while restructuring the code, as it has various conceptually similar elements (a grid with square cells that are activated by the user during gameplay, etc.)
> 
> Doing this required some rather invasive changes to the GUI code. In addition to switching to QGraphicsView, this patch also:
> - Removes the distinction between mouse and keyboard input modes. They now work together nicely, as they should.
> - Adds the ability to pause the game, and hide the puzzle while it is paused.
> - Cleans up code in various places and removes a number of obsolete class members that I presume to have been vestiges from before the KDE port.
> 
> My only disappointment is that switching to QGraphicsView did not get rid of the rendering artifacts that often appear when the cables are being rotated. Particularly on the cells with T-shaped cable junctions, white pixels often appear at the edge of the pixmap during the rotation animation. I would greatly appreciate any suggestions for how to avoid that happening.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/knetwalk/src/fielditem.cpp PRE-CREATION 
>   /trunk/KDE/kdegames/knetwalk/src/fielditem.h PRE-CREATION 
>   /trunk/KDE/kdegames/knetwalk/src/cell.cpp 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/CMakeLists.txt 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/abstractgrid.h 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/abstractgrid.cpp 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/cell.h 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/gamewidget.h PRE-CREATION 
>   /trunk/KDE/kdegames/knetwalk/src/gamewidget.cpp PRE-CREATION 
>   /trunk/KDE/kdegames/knetwalk/src/globals.h 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/knetwalk.kcfg 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/knetwalkui.rc 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/main.cpp 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/mainwindow.h 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/mainwindow.cpp 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/renderer.h 1168585 
>   /trunk/KDE/kdegames/knetwalk/src/renderer.cpp 1168585 
> 
> Diff: http://svn.reviewboard.kde.org/r/5238/diff
> 
> 
> Testing
> -------
> 
> I have come to enjoy playing this game quite a lot, and have given it considerable testing. All the usual things seem to work well including resizing, theme changes, changing difficulty level, and pausing/resuming.
> 
> 
> Thanks,
> 
> Brian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20100906/c62a184b/attachment-0001.htm

More information about the kde-games-devel mailing list