Review Request: Simplify Card Painting in KPat

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

Review request for KDE Games and Aaron Seigo.


Summary
-------

Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.

- Each Card often checks to see if it is obscured by other cards.
- Each Card has stores whether it is visible, hidden, or doesn't know.
- Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
- Only the exposed portions of a card are repainted.

I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.

Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.

This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.


Diffs
-----

  trunk/KDE/kdegames/kpat/card.h 1032448
  trunk/KDE/kdegames/kpat/card.cpp 1032448

Diff: http://reviewboard.kde.org/r/1804/diff


Testing
-------

I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.


Thanks,

Parker

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

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


Patch looks good (though I did a brief overview, not dived to deep). Mostly I wanted to tell you that there's an utility 'cpulimit' that you might find useful to emulate slow cpu. Check it in your distro packages, it should be there :)

- Dmitry


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

Dmitry Suzdalev wrote:
> Mostly I wanted to tell you that there's an utility 'cpulimit' that
> you might find useful to emulate slow cpu.

...or you could just run under valgrind ;-).

--
Matthew
Please do not quote my e-mail address unobfuscated in message bodies.
--
I sure hope they never make a hearse out of a Scion. I wouldn't want to
be caught dead in one.

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

[Albert Astals Cid-2]

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


Tried using callgrind or cachegrind to see if more or less instructions are called?

- Albert


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

> On 2009-10-08 17:54:05, Albert Astals Cid wrote:
> > Tried using callgrind or cachegrind to see if more or less instructions are called?

Argh! I did it again. Hit OK, but didn't Publish. Anyway, here's the reply I meant to leave 5 days ago.

I had considered that, but I don't really know how to get comparable data. If I had, say 30 worth of data, does more function calls mean better or worse performance? In some ways less could be better as it could be interpreted as more efficient. On the other hand, more function calls could be better as it might mean that the scene was repainted at a higher frame rate.

I'm interested in doing so profiling, but I don't have the *grind-foo to setup a sensible comparison or to properly interpret the results. If anyone has any pointers or links I'd appreciate it.


- Parker


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


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

> On 2009-10-08 09:45:29, Dmitry Suzdalev wrote:
> > Patch looks good (though I did a brief overview, not dived to deep). Mostly I wanted to tell you that there's an utility 'cpulimit' that you might find useful to emulate slow cpu. Check it in your distro packages, it should be there :)

I tried cpulimit. I had two copies of KPat (one pre patch, one post) playing the exact same game, maximised on side by side monitors, each throttled down to a measely 5% CPU and I still couldn't detect a difference. They were both choppy, but neither seemed consistently choppier than the other.

I'm not really sure if such a hacky comparison is enough grounds to throw out the optimisations.


- Parker


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


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

[Albert Astals Cid-2]

> On 2009-10-08 17:54:05, Albert Astals Cid wrote:
> > Tried using callgrind or cachegrind to see if more or less instructions are called?
>
> Parker Coates wrote:
>     Argh! I did it again. Hit OK, but didn't Publish. Anyway, here's the reply I meant to leave 5 days ago.
>    
>     I had considered that, but I don't really know how to get comparable data. If I had, say 30 worth of data, does more function calls mean better or worse performance? In some ways less could be better as it could be interpreted as more efficient. On the other hand, more function calls could be better as it might mean that the scene was repainted at a higher frame rate.
>    
>     I'm interested in doing so profiling, but I don't have the *grind-foo to setup a sensible comparison or to properly interpret the results. If anyone has any pointers or links I'd appreciate it.

Tried to callgrindize kpat a bit but results are too much imprecise because just starting and stopping can have multiple "shows" of the window giving totally different results just without your patch, sorry i can not help more :/


- Albert


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


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel

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

Ship it!


In the end, I was able to find no gains from these optimisations and they were causing problems with my port to the QtKinetic animation framework. That change does bring significant performance improvements, so I think the choice is an easy one. :)

I just applied this patch as well as the port to Q*Animation. Check out trunk for a speedy new KPat!

- Parker


On 2009-10-07 22:17:09, Parker Coates wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1804/
> -----------------------------------------------------------
>
> (Updated 2009-10-07 22:17:09)
>
>
> Review request for KDE Games and Aaron Seigo.
>
>
> Summary
> -------
>
> Lately I've been trying to clean up KPat codebase and have had a bit of success. Yesterday I had a read through the Card class and noticed that it contained lots of painting optimisations.
>
> - Each Card often checks to see if it is obscured by other cards.
> - Each Card has stores whether it is visible, hidden, or doesn't know.
> - Each Card keeps a list of cards it obscures and rechecks their visibility when it moves.
> - Only the exposed portions of a card are repainted.
>
> I found this quite surprising as Card is a subclass of QGraphicsPixmapItem and I was under the impression that the GraphicsView framework handled a lot of these things internally. I did some testing an found that this code wasn't really working as expected. If 52 cards are pile directly on top of one another, dragging a card over top of them caused all 52 of them to repaint, even though only the top one is actually visible.
>
> Of course KPat was one of the first KDE apps ported to Qt4 and GraphicsView. GraphicsView was brand new then and maybe these optimisations were needed to help it along. Since then things have changed quite a bit which may have internally broken these optimisations.
>
> This patch radically simplifies the painting logic, but I'm not necessarily sure that this is a good idea. This is more of a request for comments. It's also a request for testing, especially if someone has a trunk environment set up on a lower powered machine and could compare the perceived performance before and after this patch.
>
>
> Diffs
> -----
>
>   trunk/KDE/kdegames/kpat/card.h 1032448
>   trunk/KDE/kdegames/kpat/card.cpp 1032448
>
> Diff: http://reviewboard.kde.org/r/1804/diff
>
>
> Testing
> -------
>
> I see no regressions here, but I recently bought a new PC so I'm not sure I would notice a performance regression/improvement if there was one. When testing with QT_FLUSH_PAINT, I see roughly the same size and number of repaints both before and after the patch.
>
>
> Thanks,
>
> Parker
>
>

_______________________________________________
kde-games-devel mailing list
kde-games-devel@...
https://mail.kde.org/mailman/listinfo/kde-games-devel