2.5x blenderplayer (BGE) anti-aliasing & embedding

Patch originally submitted to Blender Tracker
http://projects.blender.org/tracker/?func=detail&aid=26799&group_id=9&atid=127

This patch is necessary for Burster web-plugin, but also for any other application relying on multisampling for blenderplayer and parented opengl window (which is currentyl supported for both Linux and OSX, and this patch adds it to Windows).


Patch from Sebastian Korczak, with some small tweaks/cleans on my end. From the tracker:

##############
This patch fix anti-aliasing (multisampling) implementation for win32 platform. It also gives opportunity to embed blenderplayer inside parent window.

Usage:
blenderplayer.exe -i 123456 -m 16 file.blend

where:
123456 - parent window handler (integer, default: 0)
16 - multisample level (integer, default: 0, max: 16. Put there maximum level you want. If not supported, player will automatically try 15,14,13,...,3,2,1)
##############

It changes ghost, so it would be nice to assure it doesn't affect Blender.

####################
Update - closed, patch committed at rev. 36470. Thanks for the review(s).

[dfelinto, 2011-04-28 17:15:20]

some comments on top of the original patch. This patch is actually the original
work from Sebastian Korczak with small cleans from me.

The patch works fine. BGE can now be embed in a .Net context :)

Things pending to be implemented as later commits:
1) expose the multisample options to the UI (in the blenderplayer panel).
2) focus/unfocus context + keyboard (to be ported from Burster patch).
3) resize of window (not working for windows at least).

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
File intern/ghost/intern/GHOST_WindowWin32.cpp (right):

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:228: int wintype =
WS_OVERLAPPEDWINDOW;
maybe to move variable declaration to top? C++ doesn't mind it, but I know it's
a good C practice.

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:548: 
We need to find a way to combine parented windows + resizing.

If we simply set the state as GHOST_kWindowStateEmbedded we will need to check
for them somewhere else. it's also strange that in Windows is the only platform
we need to make this separation.

Should it pass for the time being?

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:717: #ifdef BF_GHOST_DEBUG
GHOST_PRINTF is not working (and commented out on its declaration file). This
would be a place to use it. #ifdef BF_GHOST_DEBUG works too but :/

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:734: printf("no available pixel
format\n");
here GHOST_PRINT() can be used.

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
File source/gameengine/GamePlayer/ghost/GPG_ghost.cpp (right):

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
source/gameengine/GamePlayer/ghost/GPG_ghost.cpp:239: printf("  -i: parent
windows ID \n\n");
on windows the "parent windows ID" is called hwnd.
Should we bother adding a
#ifdef WIN: 
printf("  -i: parent windows ID (/hwnd) \n\n");
#else....

[jesterKing, 2011-04-28 21:53:25]

The patch looks good as far as I'm concerned.

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
File intern/ghost/intern/GHOST_WindowWin32.cpp (right):

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:714: while (iAttributes[17] > 0) {
iAttributes[17] looks very obscure. At least have a comment explaining it, or
better yet, a comment *and* a descriptive #define for 17

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:727: iAttributes[17] -= 1;
See the previous comment about iAttributes[17]

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
File source/gameengine/GamePlayer/ghost/GPG_Application.h (right):

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
source/gameengine/GamePlayer/ghost/GPG_Application.h:66: const bool
stereoVisual, const int stereoMode, const GHOST_TUns16 samples=0);
I know there hasn't been much before here, but please, add doxygen style docs in
headers whenever you add something :)

[dfelinto, 2011-05-04 02:01:38]

patch commited to trunk with some final small adjustments.
review #36470

(is there any option to close this issue?)

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
File intern/ghost/intern/GHOST_WindowWin32.cpp (right):

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:714: while (iAttributes[17] > 0) {
On 2011/04/28 21:53:25, jesterKing wrote:
> iAttributes[17] looks very obscure. At least have a comment explaining it, or
> better yet, a comment *and* a descriptive #define for 17
It's not that obscure. iAttributes is defined above and [17] stands for
multisampling. I think a #define would be a bit overkill so I added only a
comment :)

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:721: #ifdef BF_GHOST_DEBUG
I created GHOST_PRINTF (couldn't test though, I forgot how to enable ghost
debugging).

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:727: iAttributes[17] -= 1;
On 2011/04/28 21:53:25, jesterKing wrote:
> See the previous comment about iAttributes[17]
See the previous reply ;)

http://codereview.appspot.com/4431072/diff/1/intern/ghost/intern/GHOST_Window...
intern/ghost/intern/GHOST_WindowWin32.cpp:735: #endif // BF_GHOST_DEBUG
done

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
File source/gameengine/GamePlayer/ghost/GPG_Application.h (right):

http://codereview.appspot.com/4431072/diff/1/source/gameengine/GamePlayer/gho...
source/gameengine/GamePlayer/ghost/GPG_Application.h:66: const bool
stereoVisual, const int stereoMode, const GHOST_TUns16 samples=0);
sorry Nathan, next time? I'm still a bit clueless regarding Doxygen. I do want
to help it to be in bge code but I will do it at once. To get this patch in
trunk we must...

On 2011/04/28 21:53:25, jesterKing wrote:
> I know there hasn't been much before here, but please, add doxygen style docs
in
> headers whenever you add something :)