Fix custom program state not applied at cocos2d-x-4.0

[halx99]

No description provided.

[halx99]

see also: axmolengine/axmol#234

[badlogic]

@halx99 cheers! Could you elaborate on whether this PR supercedes #1763? From my end it looks like it does.

[halx99]

Yeah, it improve programState use, not clone per frame

[badlogic]

Thank you! I've merged this PR and did a lot of testing on it. Works great!

[rh101]

I'm getting major graphical glitches with all spine animations in my game related to these changes (with my specific use case). For example, if the textures used for each attachment are not on the same texture atlas, then the last texture used overwrites all previous textures in the different attachments. This is because the programState is assigned directly to the command, and then the uniforms in it are overwritten, causing all sorts of problems.

The programState needs to be cloned, so each attachment being rendered should have it's own copy of it, that way the uniforms set on the programState belonging to a specific attachment won't affect other attachments.

This method:

cocos2d::TrianglesCommand* SkeletonBatch::addCommand(cocos2d::Renderer* renderer, float globalOrder, cocos2d::Texture2D* texture, backend::ProgramState* programState, cocos2d::BlendFunc blendType, const cocos2d::TrianglesCommand::Triangles& triangles, const cocos2d::Mat4& mv, uint32_t flags) {
	TrianglesCommand* command = nextFreeCommand();
    const cocos2d::Mat4& projectionMat = Director::getInstance()->getMatrix(MATRIX_STACK_TYPE::MATRIX_STACK_PROJECTION);    

	if (programState == nullptr)
		programState = _programState;

	CCASSERT(programState, "programState should not be null");

	auto& pipelinePS = command->getPipelineDescriptor().programState;
	if (pipelinePS != programState) 
	{
		CC_SAFE_RELEASE(pipelinePS);
		pipelinePS = programState;
		CC_SAFE_RETAIN(pipelinePS);

		updateProgramStateLayout(pipelinePS);
	}
	
	pipelinePS->setUniform(_locMVP, projectionMat.m, sizeof(projectionMat.m));
	pipelinePS->setTexture(_locTexture, 0, texture->getBackendTexture());

    command->init(globalOrder, texture, blendType, triangles, mv, flags);
    renderer->addCommand(command);
	return command;
}

Needs to be modified to this:

cocos2d::TrianglesCommand* SkeletonBatch::addCommand(cocos2d::Renderer* renderer, float globalOrder, cocos2d::Texture2D* texture, backend::ProgramState* programState, cocos2d::BlendFunc blendType, const cocos2d::TrianglesCommand::Triangles& triangles, const cocos2d::Mat4& mv, uint32_t flags) {
	auto* command = nextFreeCommand();
    const cocos2d::Mat4& projectionMat = Director::getInstance()->getMatrix(MATRIX_STACK_TYPE::MATRIX_STACK_PROJECTION);    

	if (programState == nullptr)
		programState = _programState;

	CCASSERT(programState, "programState should not be null");

	auto& pipelinePS = command->getPipelineDescriptor().programState;
	if (pipelinePS == nullptr || pipelinePS->getProgram() != programState->getProgram())
	{
		CC_SAFE_RELEASE(pipelinePS);
		pipelinePS = programState->clone();

		updateProgramStateLayout(pipelinePS);
	}
	
	pipelinePS->setUniform(_locMVP, projectionMat.m, sizeof(projectionMat.m));
	pipelinePS->setTexture(_locTexture, 0, texture->getBackendTexture());

        command->init(globalOrder, texture, blendType, triangles, mv, flags);
        renderer->addCommand(command);
	return command;
}

For this line pipelinePS = programState;, programState should not be directly assigned to the animation, but rather a clone of it made, in case the programState is being used elsewhere with different uniform values.

EDIT: Once a clone of the programState is created, it's reference count is 1, and since it is not an autorelease object, it is not automatically freed when it's reference count gets to 1. This means that CC_SAFE_RETAIN or retain() should not be called on the cloned programState when it is assigned to the command, because when CC_SAFE_RELEASE(_programState); is called in the SkeletonBatch::~SkeletonBatch destructor, reference will go to 0, and the programState will be freed correctly.

If retain is called on it, then the reference increases to 2, and on calling CC_SAFE_RELEASE(_programState);, it'll still have a reference count of 1, so will still exist in memory without anything to free it, creating a memory leak.

[halx99]

So, if possible avoid clone per frame?

[rh101]

So, if possible avoid clone per frame?

It doesn't clone per frame, since the Program is still the same, as this condition takes care of it:

if (pipelinePS == nullptr || pipelinePS->getProgram() != programState->getProgram())

The only way it creates a clone is if the pipelinePS is null or the Program changes, which means it's an entirely different shader being used.

[halx99]

ok, your modification looks good to me

[rh101]

Do you need me to create a pull request, or do you want to take care of it?

[halx99]

Yes, just create a pr please

[badlogic]

PR merged. I'll add a multi-page atlas export sample scene for errors like these to be caught early.