(x): fix for CCRenderTexture issue.

[Panajev]

Begin and end block not restoring the view correctly.

It includes a modification to the SpriteTest demo to show the issue at hand and a way to trigger the older hackish fix I tried (simply adding an extra matrix push and pop around the render texture [rt begin][rt end] block which basically negated the modelview and projection matrices calculations at the end of the -end method of CCRenderTexture). If you add back the old -end method of CCRenderTexture, then the test will show the effect of the old "fix" (turning it off and then back on shows what happens to the sprites already added to the scene).

Right now, the old "fix" does not do a thing... same result if you leave it on or turn it off.

[Panajev]

I tested the older patch with Retina screen and found that [rt end] leaves the viewport doubled and the view appears all stretched out.

The viewport being restored asked the director the size of the window in pixels and then multiplied it by the CONTENT_SCALE_FACTOR which is how you translate points to pixels.

-(void)end
{
    CCDirector *director = [CCDirector sharedDirector];
    glBindFramebuffer(GL_FRAMEBUFFER, oldFBO_);
    CGSize size = [director winSizeInPixels];

    // restore viewport
    glViewport(0, 0, size.width * CC_CONTENT_SCALE_FACTOR(), size.height * CC_CONTENT_SCALE_FACTOR() );

    // special viewport for 3d projection + retina display
    if ( director.projection == kCCDirectorProjection3D && CC_CONTENT_SCALE_FACTOR() != 1 ) {
        glViewport(-size.width/2, -size.height/2, size.width * CC_CONTENT_SCALE_FACTOR(), size.height * CC_CONTENT_SCALE_FACTOR() );
    }

    kmGLMatrixMode(KM_GL_PROJECTION);
    kmGLPopMatrix();

    kmGLMatrixMode(KM_GL_MODELVIEW);
    kmGLPopMatrix();
}

vs

-(void)end
{
    CCDirector *director = [CCDirector sharedDirector];
    glBindFramebuffer(GL_FRAMEBUFFER, oldFBO_);
    CGSize size = [director winSize];

    // restore viewport
    glViewport(0, 0, size.width * CC_CONTENT_SCALE_FACTOR(), size.height * CC_CONTENT_SCALE_FACTOR() );

    // special viewport for 3d projection + retina display
    if ( director.projection == kCCDirectorProjection3D && CC_CONTENT_SCALE_FACTOR() != 1 ) {
        glViewport(-size.width/2, -size.height/2, size.width * CC_CONTENT_SCALE_FACTOR(), size.height * CC_CONTENT_SCALE_FACTOR() );
    }

    kmGLMatrixMode(KM_GL_PROJECTION);
    kmGLPopMatrix();

    kmGLMatrixMode(KM_GL_MODELVIEW);
    kmGLPopMatrix();
}

[ricardoquesada]

I tried your patch but it breaks the current functionality.
If you test the transition test or the rendertexture test in RetinaDisplay 4-inch, you will notice that it breaks those tests.

[Panajev]

I will look into it and fix it. Which test did fail? If it is a test using 3D projection it is expected as the proper model view matrix is not restored. Sorry riq.

[web [iPhone msg]]

On 02/ott/2012, at 02:50, Ricardo Quesada notifications@github.com wrote:

I tried your patch but it breaks the current functionality.
If you test the transition test or the rendertexture test in RetinaDisplay 4-inch, you will notice that it breaks those tests.

—
Reply to this email directly or view it on GitHub.

[Panajev]

Ok, I have tested iPhone non retina, iPhone Retina, iPhone 4'' Retina and the current version does not break the two tests you mentioned (RenderTexture test and transitions test) and keeps the improvements for the issue which was at the origin of this pull request.

[Panajev]

The behavior of begin, other than pushing the current projection and modelview matrices, is to set both matrices to their defaults values asking the director to "reset" the projection (the director will calculate both projection and modelview matrices according to the kind of projection you want... I decided to leave the value already set in the projection).

The problem with choices such as this one is that any choice you make can piss off someone, but I think this default state for begin makes sense for a RenderToTexture class for the Cocos2D project although in a very basic RenderToTexture base class you would simply reset all matrices to Identity and let the user set up the kind of projection and modelview matrices he prefers for its use case.

If one wants to use a different projection inside the CCRenderTexture begin/end block, I find it reasonable to ask the user to subclass CCRenderTexture and override the -begin method.

(Tested on: iPhone 4S, iPhone non retina simulator, iPhone retina simulator, iPhone 4'' retina simulator, iPad simulator, iPad retina simulator)

Thoughts?

[gregharding]

Panajev, I think your current fix has a problem on OSX when toggling fullscreen. Instead of being centred/offset when going fullscreen, the view resets to the bottom-left.

CCRenderTexture.m

-(void) end {
    // fbo...
    // restore viewport
    glViewport(0, 0, size.width, size.height ); // doesn't take into account fullscreen offset on OSX, see CCDirectorMac.m ~line 300
    // matrices...
}

Solution might be to cache the offset during 'begin' and apply during 'end' or call setProjection again ([director setProjection:director.projection]).

My quick fix at the moment is;

-(void) end {
    // fbo...
    // restore viewport
#if __CC_PLATFORM_IOS
    CGSize size = [director winSizeInPixels];
    glViewport(0, 0, size.width, size.height);
#elif __CC_PLATFORM_MAC
    [director setProjection:director.projection];
#endif
    // matrices...
}

[Panajev]

I could not try it on Mac because the develop-v2 branch and the branch I created from it have the Mac project broken, Xcode will not load it.
What you are saying is curious as this is exactly the problem I found on iOS yesterday morning when Riq told me the fix broke things and I did resolve that issue on iOS, tested it on device and Simulators.

I will port these changes to the master-v2 branch and see the Mac issue there.

Thanks for the report!

[web [iPhone msg]]

On 03/ott/2012, at 03:07, greg harding notifications@github.com wrote:

Panajev, I think your current fix has a problem on OSX when toggling fullscreen. Instead of being centred/offset when going fullscreen, the view resets to the bottom-left.

—
Reply to this email directly or view it on GitHub.

[gregharding]

I've got an iOS/OSX Mac project based on the cocos2d v2.1beta1 release and have been keeping them up to date with the dev branch with a few fixes here and there... (including hacking the MacDirector to use -ipadhd textures and faking CC_CONTENT_SCALE()). If you want a working base I could send one.

[Panajev]

I do not know what could have gone wrong in the repo I forked and cloned.
Actually, even cloning the cocos2d project directly and opening the Xcode
workspace on the develop-v2 branch gives me issues: it will not load the
MacOS X project...

http://i.imgur.com/tAIIX.png (Cocos2D repo, just cloned and unmodified,
develop-v2 branch, I selected the Mac xcode project... see the error)

On Wed, Oct 3, 2012 at 7:54 AM, greg harding notifications@github.comwrote:

I've got an iOS/OSX Mac project based on the cocos2d v2.1beta1 release and
have been keeping them up to date with the dev branch with a few fixes here
and there... (including hacking the MacDirector to use -ipadhd textures and
faking CC_CONTENT_SCALE()). If you want a working base I could send one.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/238#issuecomment-9096488.

[gregharding]

Yep, same here. Try opening the OSX project directly rather than the whole workspace.

[Panajev]

I checked the Mac project on the stable branch, autocreated the missing
schemes, but I do not see my modifications to CCRenderTexture making it
behave any different than other tests or the original CCRenderTexture code.

I build and run the target test (RenderTexture test) --> turn it full
screen with the OS X menu item --> scene is limited to the bottom left
quarter of the screen (same with other tests I have tried) in terms of
active area --> reset the scene --> scene is now using the full screen area
correctly.

Tried both the original code and my changes and I did not notice a
difference here (Mac-64 bit target).

On Wed, Oct 3, 2012 at 8:26 AM, greg harding notifications@github.comwrote:

Yep, same here. Try opening the OSX project directly rather than the whole
workspace.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/238#issuecomment-9096906.

[gregharding]

Ok, the problem might have already existed when going to fullscreen on OSX. If you're modifying CCRenderTexture.m anyway, could you please add a fix? The director doesn't expose the offset so we could recalculate them locally or call [director setProjection:director.projection] or... something else?

[Panajev]

Can you try to reproduce the problem with the old CCRenderTexture? Could
you send me a test case where it happens?

Also, I am not sure this depends on CCRenderTexture. Given I see the same
behaviour in Sprite test for example. The CCGL layer is indeed resized and
keeps the right color, but it looks like the scene is not re-initialized
and is not receiving any message to do so.

Switching game resolution at runtime automatically needs a bit of thought
at least.

BTW, I fixed the Xcode workspace too in the latest commit.

On Wed, Oct 3, 2012 at 8:43 AM, greg harding notifications@github.comwrote:

Ok, the problem might have already existed when going to fullscreen on
OSX. If you're modifying CCRenderTexture.m anyway, could you please add a
fix? The director doesn't expose the offset so we could recalculate them
locally or call [director setProjection:director.projection] or...
something else?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/238#issuecomment-9097122.

[Panajev]

Also, your fix did not seem to have any effect that I noticed sadly.

[gregharding]

Really? When I use that fix it centres properly when going OSX fullscreen, without it the view aligns bottom-left. You might need to clean your build or something, perhaps. Email me and I'll try to reply with a test-case etc.

[Panajev]

I ahve uploaded two pics of the Menu test here: normal window and full-screen.

http://imgur.com/a/75c6M

The issue of the view not taking advantage of full-screen/increased resolution/viewport not being centered (unless we restarted the scene or clicked a menu button, then it gets all set) is not related directly to CCRenderTexture as you can see:

[gregharding]

Looks good at a glance - will test it as soon as I can.

For anyone reading this thread, the fullscreen centring issue on OSX occurs when the AppDelegate uses resize mode kCCDirectorResize_AutoScale;

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification
    // ...
    [director_ setResizeMode:kCCDirectorResize_AutoScale];

So, when using a feature like CCRenderTexture which alters the viewport settings to render into a texture, currently after [renderTexture end] is called the viewport does not restore correctly. Panajev's updates for CCRenderTexture and CCDirectorIOS/Mac should fix the existing rendertexture issue (related to content scale factor) and restore the viewport correctly when using fullscreen OSX with AutoScale enabled.

[ricardoquesada]

@Panajev:
Regarding the issues with the develop-v2 branch, don't forget to update the submodules:
$ git submodule update --init

[Panajev]

@riq: thank you for the reminder... I have been bit by the submodules bug
:(. Is the move to git submodules going to be in the master-v2 branch. I
have not encountered troubles with regards to submodules.

Should I update the submodules in the current branch for the fix or take
this as a future reminder? I would not want to risk losing commits.

On Sat, Oct 6, 2012 at 12:25 AM, Ricardo Quesada
notifications@github.comwrote:

@Panajev https://github.com/Panajev:
Regarding the issues with the develop-v2 branch, don't forget to update
the submodules:

$ git submodule update --init

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/238#issuecomment-9191713.

[ricardoquesada]

@Panajev:
master-v2 for the moment it is not using submodules, but it will have submodules once I merge the develop-v2 changes into it.
It is not necessary to update your branch with the submodules, but if you do it, it is fine too.
I will work your patch tomorrow... Thanks.

[ricardoquesada]

update: I won't be able to work on it today. I will continue to work on the CCRenderTexture pull requests this coming Monday.

[Panajev]

@ricardoquesada: Hey Riq, I made some further commits which fixed an issue if you compiled the modified test under OS X and fixed a comment in ccConfig.h related to CC_DIRECTOR_IOS_USE_BACKGROUND_THREAD.

[ricardoquesada]

@Panajev thanks.

[Panajev]

@ricardoquesada would you like me to update this branch to the latest upstream changes?
Some changes that are here, in addition to the RenderTexture files ones, are related to the Xcode project and the changes for the test related to the issue I wanted to fix. Also, I enabled back THUMB code generation for AMRv7 and ARMv7S targets (not for AMRv6) where the old thumb+VFP/NEON switches ar enot an issue any longer and we can reap benefits from reduced code size (which is why we use -Os instead of -O3 for release builds).

[gregharding]

I noticed cocos v2.1 beta 3 doesn't have these fixes yet... hope they haven't been forgotten :) Panajev@3b0cbb3

eg. the new CCRenderTexture.m still has this, which looks wrong;

-(void)end
{
    // ...

    CGSize size = [director winSizeInPixels];

    // restore viewport
    glViewport(0, 0, size.width * CC_CONTENT_SCALE_FACTOR(), size.height * CC_CONTENT_SCALE_FACTOR() );

    // ...
}

If I get time I'll check the new beta3 with the Mac fullscreen viewport restoration problems I was having after using rendertextures, unless you're still going to look at these fixes, @ricardoquesada

[ricardoquesada]

@mb1
I have not forgotten it :)
I was busy with other features. I will try to include in beta4... (or in rc0 if I am not able to include it in beta4)

[ricardoquesada]

@Panajev
Sorry for the delay. I started playing again with your patch.
I'm testing your test case, but it seems that with or without the patch the test case has the same behavior. Do you have any idea ?
Or what would be another easy way to reproduce the issue ? thanks

[Panajev]

Hi @ricardoquesada,

Unless you have already applied changes which make the patch less effective or something else came up, I am a bit at a loss of words :(. Can you tell me more about how the test case operates with and without the patch? Fails both times? Passes both times?

I will try to take a look asap, but my wife and I are moving to the U.K. right now and I have not had time to run the PC on in a few days, looking for a house and all.

Keep up the great work :), Cocos2D and the new JS bindings hold a LOT of promise!!!!

[web [iPhone msg]]

On 10/dic/2012, at 23:25, Ricardo Quesada notifications@github.com wrote:

@Panajev
Sorry for the delay. I started playing again with your patch.
I'm testing your test case, but it seems that with or without the patch the test case has the same behavior. Do you have any idea ?
Or what would be another easy way to reproduce the issue ? thanks

—
Reply to this email directly or view it on GitHub.

[ricardoquesada]

Hi @Panajev
What I did was to test your repo
$ git reset --hard Panajev/develop-v2-matrixfix

And run the SpriteTest in Mac.
I tried it both in full screen and non-full screen, and with the "enable fix" enabled and disabled and the results were the same.

What should I see when the fix is enabled ?
Thanks.

ps: All the best in finding a home.

[gregharding]

Is this the viewport bug where you toggle back and forth from fullscreen and the view resets to the bottom left instead of being centred?

[ricardoquesada]

@mb1: I guess... what I am trying to do is to reproduce the bug, so the test case that is included in this patch has a button that says "enable fix". I tried running the test without pressing the button, and also pressing the button, both in fullscreen and non-fullscreen and I think the results were the same.
What should be the expected behavior if I don't press "enable fix" ? Thanks.

[gregharding]

I never tested @Panajev's fixes separately but I did patch a version of cocos we're using with bits of them to enable the fullscreen toggling to work without messing up the viewport. In the end the render-to-texture stuff wasn't working correctly so we removed it. From memory I was just going fullscreen and then rendering something to texture to cause the viewport problem. I haven't looked at the test submitted in this patch, but the fix I saw separated the viewport setup into another method which was then called at the end of rendering to texture, I think.

[ricardoquesada]

@mb1 thanks.
What would be the easiest way to reproduce this bug ?
If I run the "RenderTexture" test on Mac, a switch from non-fullscreen to fullscreen... show that trigger the bug ? If so, what should happen ? thanks.

[gregharding]

@ricardoquesada I'll just build the tests for the latest cocos 2.1-beta4 and let you know what I'm seeing...

[Panajev]

Hello,

From memory this patch started because when dealing with a CCRenderTexture and updating it inside a custom draw method the viewport and the projection transform might not be correctly setup and the object would not appear where it should be, but in the lower left corner of the screen. Initially the button would enable and disable the "fix" I had been testing, it used to be an extra push and pop of all matrices, but the it evolved into something more.

It might very well be that the current button does not do a thing anymore because the core of the issue has been fixed by the rest of my patch (basically it being redundant), so it is an extra check to see if things change or not when testing changes to the patch, but as I am saying I am talking from memory.

What you would have seen a long while ago would have been the sprite off center when the fix was not applied, but as I said the situation changed as my patch digger deeper and changed more things not only in CCRenderTexture but in the iOS and Mac directors.

Again, sorry about this. I will dig the forum posts related to the issue as son as possible.

Thanks for the well wishes Riq. We should have found a good house, but we cannot move in for a a week or two as the place is being renovated. The previous tenant wrecked the place... Oh well... :).

Sent from my iPad

On 12/dic/2012, at 06:51, greg harding notifications@github.com wrote:

@ricardoquesada I'll just build the tests for the latest cocos 2.1-beta4 and let you know what I'm seeing...

—
Reply to this email directly or view it on GitHub.

[ricardoquesada]

@Panajev
Thanks. So, if I run the test that is in your patch, without applying the patch I should trigger the bug, is that correct ?

[ricardoquesada]

Merged, thanks.
For some reason the patch broke the "auto draw" feature that I added a few weeks ago, but I will fix it.