OGRECave / ogre

scene-oriented, flexible 3D engine (C++, Python, C#, Java)
https://ogrecave.github.io/ogre/
MIT License
3.89k stars 960 forks source link

Two separate multisampled FBO's get the same renderbuffer due to caching, both FBO's contents are combined as a result #3158

Closed FrankGoyens closed 1 month ago

FrankGoyens commented 1 month ago

System Information

Detailled description

Hello, I encountered a problem when using multisampled FBO's.

I have two textures of the same size onto which I render different content using a "render to texture" FBO. The problem is that the contents of the first texture and the contents of the second texture are unintentionally combined, because they internally use the same OpenGL renderbuffer. They use the same buffer because the GLFBOManager::requestRenderBuffer uses a caching mechanism based on: format, width, height and nr of samples, which is the same for both textures.

This is likely only a problem when "clearEveryFrame" is false. My rendering setup has a mechanism that requires "clearEveryFrame" to be false for both rendertargets, if "clearEveryFrame" is true then the contents wouldn't be combined as the unintentionally shared buffer gets cleared beforehand.

At my applications' side, I use Ogre::TextureManager::getSingleton().createManual(GLNameGenerator::Generate(), Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME, Ogre::TEX_TYPE_2D, width, height, 0, Ogre::PF_R8G8B8A8, Ogre::TU_RENDERTARGET, 0, false, 8); to create a texture with a renderTarget. And within the OGRE library it isn't until the GLFrameBufferObject::initialise, where on line 147 mMultisampleColourBuffer = mManager->requestRenderBuffer(format, width, height, mNumSamples); is called.

So I wonder how this problem could be solved? I would try to solve this myself, but I think there's multiple approaches, so I could use some input.

paroj commented 1 month ago

sounds like https://github.com/OGRECave/ogre/issues/3015

FrankGoyens commented 1 month ago

Thank you for the quick response.

That seems like the same cause indeed. If I understand correctly, in the other issue they initially called update() on the viewports, and the solution was to call update() on the renderer as well?

In our application we call renderer->update() for both rendertarget textures, they each have one viewport, and the viewports are set to auto update, so renderer->update() triggers the viewport's update(). I tried setAutoUpdated(false) on the viewports, then the textures remain blank, which is expected I guess because we don't directly call update() on the viewport.

Note that both rendertarget textures have clearEveryFrame=false in my scenario, the (unintentionally) shared RenderBuffer doesn't get cleared between updating the first and second rendertarget. In the other issue clearEveryFrame is left default to true, which prevents the problem.

I think the two rendertarget textures shouldn't use the same RenderBuffer in my scenario. The solution would be to somehow prevent mapping these to textures onto the same cache item (adding a parameter perhaps?), or prevent RenderBuffer cache use for an FBO altogether?

paroj commented 1 month ago

ah.. I see.. well the first problem is to check the clearEveryFrame setting in the FBO. I guess RenderSystem::_getViewport will do here.

The next issue is that clearEveryFrame is a dynamic property (in contrast to e.g. the resolution). So we must dynamically switch to a cached RenderBuffer if it becomes true again.

Maybe it is easier to copy the contents from mFB back to mMultisampleFB if clearEveryFrame=false? Just reversing what swapBuffers is doing.

FrankGoyens commented 1 month ago

I tried some things based on your feedback.

Since clearEveryFrame is a dynamic property which is set with a simple setClearEveryFrame function, and it should trigger re-initialisation of the FBO, I figured a good place to apply this property to the FBO is GLFrameBufferObject::bind.

Using Root::getSingleton().getRenderSystem()._getViewport() I checked whether clearEveryFrame is different from when the previous GLFrameBufferObject::initialise was called, and if the value is different I call GLFrameBufferObject::initialise again.

GLFrameBufferObject::initialise would then either create a RenderBuffer using the existing mManager->requestRenderBuffer(format, width, height, mNumSamples); or a new function that creates a RenderBuffer without caching it.

This didn't work however, because calling _getViewport in the bind function gave the wrong viewport. The correct viewport wasn't made active yet, apparently binding the rendertarget happens before setting the active viewport in the rendersystem.

So I looked for a more robust way of determining whether the cache should be used in an FBO initialise based on clearEveryFrame. My idea is to let the renderTarget (GLFBORenderTexture) determine this, this is not trivial since there could be multiple viewports per renderTarget. But as soon as there is at least one clearEveryFrame=false viewport in the renderTarget, the FBO shouldn't use the cache anymore. Using that reasoning, GLFBORenderTexture, the FBO's owner, could manage this aspect of the FBO. That's my reasoning at least, how does that sound to you?

And about your other suggestion to copy mFB back to mMultisampleFB, that could work, but I am a little concerned about performance. Copying backwards would be necessary every time two clearEveryFrame=false viewports are updated in succession, which is likely what we would do in our setup. Besides you would still need a robust way of checking clearEveryFrame from within the FBO.

paroj commented 1 month ago

as soon as there is at least one clearEveryFrame=false viewport in the renderTarget, the FBO shouldn't use the cache anymore.

this sounds like a good approach!

And about your other suggestion to copy mFB back to mMultisampleFB, that could work, but I am a little concerned about performance.

I dont think the performance impact would be noticable, however probably this copy direction is not well tested on OpenGL drivers and might break. The condition for this would be the same though: at least one clearEveryFrame=false viewport.

So it should be quick to try both approaches once you get to the clearEveryFrame condition.