{"action":"created","issue":{"url":"https://api.github.com/repos/cinder/Cinder/issues/1191","labels_url":"https://api.github.com/repos/cinder/Cinder/issues/1191/labels{/name}","comments_url":"https://api.github.com/repos/cinder/Cinder/issues/1191/comments","events_url":"https://api.github.com/repos/cinder/Cinder/issues/1191/events","html_url":"https://github.com/cinder/Cinder/issues/1191","id":116923687,"number":1191,"title":"gl::Fbo and layered rendering (or 3d attachments)","user":{"login":"simongeilfus","id":241098,"avatar_url":"https://avatars.githubusercontent.com/u/241098?v=3","gravatar_id":"","url":"https://api.github.com/users/simongeilfus","html_url":"https://github.com/simongeilfus","followers_url":"https://api.github.com/users/simongeilfus/followers","following_url":"https://api.github.com/users/simongeilfus/following{/other_user}","gists_url":"https://api.github.com/users/simongeilfus/gists{/gist_id}","starred_url":"https://api.github.com/users/simongeilfus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/simongeilfus/subscriptions","organizations_url":"https://api.github.com/users/simongeilfus/orgs","repos_url":"https://api.github.com/users/simongeilfus/repos","events_url":"https://api.github.com/users/simongeilfus/events{/privacy}","received_events_url":"https://api.github.com/users/simongeilfus/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-11-14T13:06:49Z","updated_at":"2015-12-01T15:46:10Z","closed_at":null,"body":"Hey all,\r\nThere's a few small changes we should do to properly support that type of attachments in our ```Fbo``` class. At the moment trying to attach a ```GL_TEXTURE_3D``` will result in a ```GL_INVALID_ENUM``` error (and from the code, I would say the same will happen when trying to attach a ```GL_TEXTURE_1D```). This will unfortunately not be caught as a ```FboException``` and will fail silently if not checking for gl errors. Which is definitely bad!\r\n\r\nSnippet that trigger the error:\r\n```c++\r\n{\r\n\tauto tex3d\t\t= gl::Texture3d::create( 128, 128, 128, texFormat );\r\n\tauto fboFormat\t= gl::Fbo::Format().attachment( GL_COLOR_ATTACHMENT0, tex3d );\r\n\ttry {\r\n\t\tmFbo\t\t= gl::Fbo::create( 128, 128, fboFormat );\r\n\t}\r\n\tcatch( const ci::Exception &exc ) { CI_LOG_EXCEPTION( \"Fbo init\", exc ); }\r\n\t\r\n\tCI_CHECK_GL(); // GL_INVALID_ENUM\r\n}\r\n```\r\n\r\nThere's two approach for a fix. The first one is to check for the texture target when attaching textures:\r\n\r\n```c++\r\nvoid Fbo::attachAttachments()\r\n{\r\n\t// attach Renderbuffers\r\n\tfor( auto &bufferAttachment : mAttachmentsBuffer )\r\n\t\tglFramebufferRenderbuffer( GL_FRAMEBUFFER, bufferAttachment.first, GL_RENDERBUFFER, bufferAttachment.second->getId() );\r\n\t\r\n\t// attach Textures\r\n\tfor( auto &textureAttachment : mAttachmentsTexture ) {\r\n\t\tauto textureTarget = textureAttachment.second->getTarget();\r\n\t\tif( textureTarget == GL_TEXTURE_1D ) {\r\n\t\t\tglFramebufferTexture1D( GL_FRAMEBUFFER, textureAttachment.first, textureTarget, textureAttachment.second->getId(), 0 );\r\n\t\t}\r\n\t\telse if( textureTarget == GL_TEXTURE_2D ) {\r\n\t\t\tglFramebufferTexture2D( GL_FRAMEBUFFER, textureAttachment.first, textureTarget, textureAttachment.second->getId(), 0 );\r\n\t\t}\r\n\t\telse if( textureTarget == GL_TEXTURE_CUBE_MAP ) {\r\n\t\t\ttextureTarget = GL_TEXTURE_CUBE_MAP_POSITIVE_X;\r\n\t\t\tglFramebufferTexture2D( GL_FRAMEBUFFER, textureAttachment.first, textureTarget, textureAttachment.second->getId(), 0 );\r\n\t\t}\r\n\t\telse if( textureTarget == GL_TEXTURE_3D ) {\r\n\t\t\tglFramebufferTexture3D( GL_FRAMEBUFFER, textureAttachment.first, textureTarget, textureAttachment.second->getId(), 0, 0 );\r\n\t\t}\r\n\t}\t\r\n}\r\n```\r\n\r\nBut apparently the GL3/GL4 way of doing this would be to use this instead:\r\n***EDIT: Actually this should be the preferred fix, as it is the only one that allows the use of ```gl_Layer``` in a geometry shader.***\r\n***EDIT2: Should add ```#ifdefs``` for ES2 and 3 as this is not supported on mobile.***\r\n\r\n```c++\r\nvoid Fbo::attachAttachments()\r\n{\r\n\t// attach Renderbuffers\r\n\tfor( auto &bufferAttachment : mAttachmentsBuffer )\r\n\t\tglFramebufferRenderbuffer( GL_FRAMEBUFFER, bufferAttachment.first, GL_RENDERBUFFER, bufferAttachment.second->getId() );\r\n\t\r\n\t// attach Textures\r\n\tfor( auto &textureAttachment : mAttachmentsTexture ) {\r\n\t\tauto textureTarget = textureAttachment.second->getTarget();\r\n\t\tif( textureTarget == GL_TEXTURE_CUBE_MAP )\r\n\t\t\ttextureTarget = GL_TEXTURE_CUBE_MAP_POSITIVE_X;\r\n\t\tglFramebufferTexture( GL_FRAMEBUFFER, textureAttachment.first, textureAttachment.second->getId(), 0 );\r\n\t}\t\r\n}\r\n```\r\n\r\nIf we were to add support for this type of Fbo rendering we should also add this to the Fbo status check :\r\n```c++\r\n\t\tcase GL_FRAMEBUFFER_INCOMPLETE_LAYER_TARGETS:\r\n\t\t*resultExc = FboExceptionInvalidSpecification( \"Framebuffer incomplete: not all attached images are layered\" );\r\n\t\treturn false;\r\n```\r\n\r\nAs the above code snippet would result in an incomplete framebuffer. For some reason this happen only when using the glFramebufferTexture variant. But basically what happen in that case is that we're trying to attach a layered texture (GL_TEXTURE_3D) with a non-layered depth buffer (which is added by default). \r\n\r\nSo creating a Fbo with a 3d attachment should specify that as well (or maybe we could a check to detect this when creating the default depth attachment):\r\n\r\n```c++\r\nauto fboFormat = gl::Fbo::Format().attachment( GL_COLOR_ATTACHMENT0, tex3d ).disableDepth();\r\n```\r\n\r\nThinking about all this raise a few other questions:\r\n* Does it still make sense to only have ( width, height ) constructors? \r\n* Should the ```gl::Fbo``` class do like ```gl::TextureData``` and have all 3 dimensions no matter which one are used?\r\n* IMO the above is the perfect occasion to think about adding attach/detach methods to the ```Fbo``` class. Currently we have to destroy and re-create an fbo if we want to change its attachments. Which is a shame as ```glFramebufferTexture``` and ```glFramebufferRenderbuffer``` are in the end the same as any other opengl binding functions.\r\n* I have not really thought about the implications when multisampling is on, ..."},"comment":{"url":"https://api.github.com/repos/cinder/Cinder/issues/comments/161006213","html_url":"https://github.com/cinder/Cinder/issues/1191#issuecomment-161006213","issue_url":"https://api.github.com/repos/cinder/Cinder/issues/1191","id":161006213,"user":{"login":"andrewfb","id":227107,"avatar_url":"https://avatars.githubusercontent.com/u/227107?v=3","gravatar_id":"","url":"https://api.github.com/users/andrewfb","html_url":"https://github.com/andrewfb","followers_url":"https://api.github.com/users/andrewfb/followers","following_url":"https://api.github.com/users/andrewfb/following{/other_user}","gists_url":"https://api.github.com/users/andrewfb/gists{/gist_id}","starred_url":"https://api.github.com/users/andrewfb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andrewfb/subscriptions","organizations_url":"https://api.github.com/users/andrewfb/orgs","repos_url":"https://api.github.com/users/andrewfb/repos","events_url":"https://api.github.com/users/andrewfb/events{/privacy}","received_events_url":"https://api.github.com/users/andrewfb/received_events","type":"User","site_admin":false},"created_at":"2015-12-01T15:46:10Z","updated_at":"2015-12-01T15:46:10Z","body":"Hey Simon - I've read through this and follow what you're saying, but you're ahead of me on the relevant deeper reading.\r\n\r\nThe default addition of depth to `Fbo::Format` was motivated by the common use case, but I can see how it complicates the advanced one. The 2 approaches you presented above do seem like reasonable concessions to make for allowing the advanced use case w/o complicating the simple one.\r\n\r\nThe multisample texture storage bits seem nastier but also probably unavoidable. I do agree that `allocateTexStorage2dMultisample()` et al is probably the right direction to take this. Unfortunately I don't have any insight into that confusing portion of the Framebuffer docs at the moment.\r\n\r\nDo we think the `gl::FboCubeMap` class still has a place for simplifying that particular case, even if it's primarily a wrapper around this more powerful FBO?\r\n\r\nOn the one hand, all of this feels like it's complicating an already complicated class. On the other hand, I don't think these changes will complicate the simple use case per se. Can we come up with a good sample to demonstrate and test layered rendering? At this point I'd be inclined to just see some experimenting and we'll judge whether it's an unnecessary complication or not from that..."}}