Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering Process Refactoring - FBO creation #1668

Merged
merged 1 commit into from
Apr 17, 2015
Merged

Rendering Process Refactoring - FBO creation #1668

merged 1 commit into from
Apr 17, 2015

Conversation

emanuele3d
Copy link
Contributor

This is the first of a number of PR aiming to refactor the class LwjglRenderingProcess.

The destination I am aiming for (minus feedback and some changes that have already taken place in this PR) is available in this branch. While there are no functional changes (the look and performance of Terasology's renderer should stay the same) the changes to the code are substantial, primarily in the interest of readability, maintainability and, eventually, ease of adding new capabilities. For this reason I won't submit that branch as a PR, only as a reference of where I'm heading.

Now, some background info: OpenGL's FrameBuffers can be thought of boxes with a number of internal slots (attachment points). Once something is placed in one of those slots (is attached to the FrameBuffer's attachment point), it becomes available, among other things, to shaders to read from or write to. Oversimplifying, a shader might read from slot A, containing an image, and produce a blurred version of it in slot B.

To start small(ish) this PR focuses only on the code generating OpenGL FrameBuffers and their attachments, which are then wrapped and manipulated through FBO instances. The original FBO-generating code was in a single LwjglRenderingProcess.createFBO() method. This code has been restructured into (public) FBO.create() method and a number of (private) support methods.

Also, as originally suggested by @immortius long time ago, an LwjglRenderingProcess.FBObuilder class, is now available, making the FBO instantiation syntax a little easier on the eye.

Finally, javadocs and some comments have been added throughout the new and affected code.

* now includes a Type and Status enums, storage for the status and a getStatus() method
* added attachDepthBufferTo() and dispose() methods, originally in LwjglRenderingProcess
* added width(), height() and getDimensions() methods
* added the all-important create() public method (and a number of private support methods) to actually generate a FrameBuffer and its attachments on the GPU.

LwjglRenderingProcess class:
* added an FBObuilder (inner) class making the creation of FBOs a more readable affair
* all instances of FBO creation now take advantage of the new builder
* moved FBOType enum to FBO.Type
* removed createFBO methods (all the functionality has gone in the FBO class)

Added javadocs and comments throughout the new code.
@GooeyHub
Copy link
Member

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/20/
Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

I've given it a look (woohoo clear documentation and cleanup! I nearly feel like I understand it now!) and tested locally in my mega-workspace, no noted issues :-)

Will merge tomorrow if no other feedback by then (ping @immortius, @OvermindDL1, etc, anybody interested)

@emanuele3d
Copy link
Contributor Author

Thanks Cervator! I was actually thinking of submitting a new commit without the comments as they basically double the amount of lines to review. I thought perhaps I should have added them at a later stage as I thought originally, to focus on the code instead. But I'm glad at last you appreciate them. =)

@emanuele3d
Copy link
Contributor Author

Whoops. I just noticed the link to the destination I'll be aiming for over the next weeks, in the initial description of his PR, was wrong. It is now correct and brings here: https://github.com/emanuele3d/Terasology/tree/RendererChangesBKP

@Cervator
Copy link
Member

No way on separate comments, they're great and we need more of them :) I enjoy being able to understand this stuff better, hehe

@LinusVanElswijk
Copy link
Member

Nice work :) Love the documentation

@Cervator Cervator merged commit a29e3f2 into MovingBlocks:develop Apr 17, 2015
Cervator added a commit that referenced this pull request Apr 17, 2015
@emanuele3d
Copy link
Contributor Author

Yay! Thanks for the merge @Cervator! And thanks for the kind words @LinusVanElswijk !

@emanuele3d emanuele3d deleted the ImproveFBOclassAndAddFBOBuilder branch April 17, 2015 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants