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

Nodes use FBOConfig to add their desired FBOs to FBM #2431

Merged
merged 37 commits into from Aug 19, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Aug 4, 2016

Contains

Initial but working PR for the issue: Nodes generate and publish the FBOs they need, unless available already. All current-default nodes in this PR have requireFBO statements.

How to test

Please check any weirdness on rendering in the PR. Sorry for not specifying any particular rendering operation since all nodes were modified.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 4, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 4, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 4, 2016

Member

Hehe, yep, that import was the weird one that had actually latched on to a dependency from the PC facade in the past, finally fixed and cleaner now :-)

Member

Cervator commented Aug 4, 2016

Hehe, yep, that import was the weird one that had actually latched on to a dependency from the PC facade in the past, finally fixed and cleaner now :-)

@@ -527,6 +534,13 @@ public Dimensions dividedBy(int divisor) {
return new Dimensions(width / divisor, height / divisor);
}
public Dimensions multiplyBy(float multiplier) {

This comment has been minimized.

@msteiger

msteiger Aug 5, 2016

Member

Maybe move this method to Dimensions?

@msteiger

msteiger Aug 5, 2016

Member

Maybe move this method to Dimensions?

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 5, 2016

Contributor

Commencing review... ta-ta-ta-dah!!

Contributor

emanuele3d commented Aug 5, 2016

Commencing review... ta-ta-ta-dah!!

@@ -28,6 +28,9 @@
void process();
// TODO: invoked when Node is removed from RenderGraph
void dispose();

This comment has been minimized.

@emanuele3d

emanuele3d Aug 5, 2016

Contributor

Warning: for what I have seen in the codebase dispose() is called to give a chance to an object to do cleanup before it is destroyed and removed from memory. At this stage my thinking is that a node might continue to live even if it's not in the render graph. There are caveats to be discussed about this but I think they should be discussed within the context of the functionality to add/remove nodes to the graph and add/remove connections between them.

@emanuele3d

emanuele3d Aug 5, 2016

Contributor

Warning: for what I have seen in the codebase dispose() is called to give a chance to an object to do cleanup before it is destroyed and removed from memory. At this stage my thinking is that a node might continue to live even if it's not in the render graph. There are caveats to be discussed about this but I think they should be discussed within the context of the functionality to add/remove nodes to the graph and add/remove connections between them.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 6, 2016

Contributor

Dear @tdgunes I have now reviewed all the new code.

I guess the general comment is: this PR doesn't go far enough. 😃

Let's take a step back and let's talk in broader terms what we are trying to achieve here. First of all, what are the important characteristics of an FBO? Its dimensions and type are the most obvious that come to mind. It can then have a number of possible attachments on top of the default (but not always present) color buffer: normals buffer, light buffer, depth+stencil buffer.

There are also more conceptual but no less important characteristics: some buffers come in pairs, so that one can be used for reading while the other gets used for writing, ready to be swapped when necessary. Some attachments are shared, i.e. sceneOpaque and sceneReflectiveRefractive share the depth+stencil buffer. Some FBOs are used by most nodes, some FBOs are used only by a few nodes, sometimes just one.

Finally, some buffers are regenerated in specific circumstances (but not necessarily the same circumstances for all FBOs) and some buffers don't change for the whole life of the renderer.

Now, when we look at the classes dealing with the FBOs, what do we have? Currently we have an omniscient FBM knowing more or less everything about FBOs. Then we have highly constrained Nodes: they have to work within the limits of what the FBM can do for them.

This is not ideal.

Ideally, the FBM knows very little. Ideally the Nodes are the kings that tell the FBM what to do. This way when modules add new nodes the FBM works for them, not viceversa.

Now, how do we achieve this? One way could be to make the FBM extremely dumb: just a "bullettin board" where Nodes "publish" their FBOs so that other nodes can find them and reuse them. The problem with this vision is that the FBM actually does one very useful thing: it can act as a single observer of the RenderingConfig regenerating the FBOs as needed and informing the nodes when necessary. The alternative would be for each node to monitor the RenderingConfig and they'd all have to find a way to coordinate with each other, to avoid attempting to regenerate each FBO multiple times. Obviously this is not a solution. On the other hand, the FBM as it is has to go through hoops to handle the special cases (shadow map, static buffers) and can't handle anything different if a module's node needed it.

Let me then introduce the broad strokes of a solution.

Have a BaseFBM and a number of specialized FBMs: (currently) one for display-size-dependent FBOs, one for shadow map resolution-dependent FBOs and one for static FBOs. Nodes register their FBOs with the specialized FBMs, not the base one. Nodes could potentially also register their own specialized FBM if they need something different. Specialized FBMs will then be in charge of regenerating the FBOs when/if necessary and informing the subscribed BindFBO state changes so that they can obtain the new fboIDs. Note that in this scenario FBMs do not need to know the FBOs they will hold. As each FBO instance in a specialized FBM can be treated like all others, specialized FBMs are just a slightly smarter <String, FBO> map.

Some additional notes:

  1. the display-size-dependent FBM should provide three default FBOs: a "ReadOnlyGBuffer", a "WriteOnlyGBuffer" (formerly sceneOpaque/pingpong) and a "FinalImage" buffer (formerly sceneFinal).
  2. all other FBOs should be generated by the nodes that need them
  3. the display-size-dependent FBM should also handle FBO regeneration during screenshots.
  4. FBMs should provide the functionality to swap two FBOs and consequentially trigger a round of notifications, but only if the two FBO's characteristics are identical.
  5. BindFBO constructors will need a new FBM parameter, so that FBOs can be obtained from the right FBM.
  6. buffer-sharing betwen FBOs will probably need to be handled as it is now, within Node.process().

Hmmm... 3:22am, fatigue is starting to set in. Let me close this comment here then. I have concerns about the whole FBOConfig architecture and usage as to me they should be transient entities, used only to generate the FBO but not retained or further manipulated. But let's leave them as they are and please focus only on the feedback I gave you in this comment.

I hope to attend at the event tomorrow afternoon. I hope you'll attend to so that we can discuss things further.

Contributor

emanuele3d commented Aug 6, 2016

Dear @tdgunes I have now reviewed all the new code.

I guess the general comment is: this PR doesn't go far enough. 😃

Let's take a step back and let's talk in broader terms what we are trying to achieve here. First of all, what are the important characteristics of an FBO? Its dimensions and type are the most obvious that come to mind. It can then have a number of possible attachments on top of the default (but not always present) color buffer: normals buffer, light buffer, depth+stencil buffer.

There are also more conceptual but no less important characteristics: some buffers come in pairs, so that one can be used for reading while the other gets used for writing, ready to be swapped when necessary. Some attachments are shared, i.e. sceneOpaque and sceneReflectiveRefractive share the depth+stencil buffer. Some FBOs are used by most nodes, some FBOs are used only by a few nodes, sometimes just one.

Finally, some buffers are regenerated in specific circumstances (but not necessarily the same circumstances for all FBOs) and some buffers don't change for the whole life of the renderer.

Now, when we look at the classes dealing with the FBOs, what do we have? Currently we have an omniscient FBM knowing more or less everything about FBOs. Then we have highly constrained Nodes: they have to work within the limits of what the FBM can do for them.

This is not ideal.

Ideally, the FBM knows very little. Ideally the Nodes are the kings that tell the FBM what to do. This way when modules add new nodes the FBM works for them, not viceversa.

Now, how do we achieve this? One way could be to make the FBM extremely dumb: just a "bullettin board" where Nodes "publish" their FBOs so that other nodes can find them and reuse them. The problem with this vision is that the FBM actually does one very useful thing: it can act as a single observer of the RenderingConfig regenerating the FBOs as needed and informing the nodes when necessary. The alternative would be for each node to monitor the RenderingConfig and they'd all have to find a way to coordinate with each other, to avoid attempting to regenerate each FBO multiple times. Obviously this is not a solution. On the other hand, the FBM as it is has to go through hoops to handle the special cases (shadow map, static buffers) and can't handle anything different if a module's node needed it.

Let me then introduce the broad strokes of a solution.

Have a BaseFBM and a number of specialized FBMs: (currently) one for display-size-dependent FBOs, one for shadow map resolution-dependent FBOs and one for static FBOs. Nodes register their FBOs with the specialized FBMs, not the base one. Nodes could potentially also register their own specialized FBM if they need something different. Specialized FBMs will then be in charge of regenerating the FBOs when/if necessary and informing the subscribed BindFBO state changes so that they can obtain the new fboIDs. Note that in this scenario FBMs do not need to know the FBOs they will hold. As each FBO instance in a specialized FBM can be treated like all others, specialized FBMs are just a slightly smarter <String, FBO> map.

Some additional notes:

  1. the display-size-dependent FBM should provide three default FBOs: a "ReadOnlyGBuffer", a "WriteOnlyGBuffer" (formerly sceneOpaque/pingpong) and a "FinalImage" buffer (formerly sceneFinal).
  2. all other FBOs should be generated by the nodes that need them
  3. the display-size-dependent FBM should also handle FBO regeneration during screenshots.
  4. FBMs should provide the functionality to swap two FBOs and consequentially trigger a round of notifications, but only if the two FBO's characteristics are identical.
  5. BindFBO constructors will need a new FBM parameter, so that FBOs can be obtained from the right FBM.
  6. buffer-sharing betwen FBOs will probably need to be handled as it is now, within Node.process().

Hmmm... 3:22am, fatigue is starting to set in. Let me close this comment here then. I have concerns about the whole FBOConfig architecture and usage as to me they should be transient entities, used only to generate the FBO but not retained or further manipulated. But let's leave them as they are and please focus only on the feedback I gave you in this comment.

I hope to attend at the event tomorrow afternoon. I hope you'll attend to so that we can discuss things further.

@tdgunes tdgunes changed the title from [RFR] Nodes use FBOConfig to add their desired FBOs to FBM to [WIP] Nodes use FBOConfig to add their desired FBOs to FBM Aug 9, 2016

@tdgunes tdgunes changed the title from [WIP] Nodes use FBOConfig to add their desired FBOs to FBM to [RFR] Nodes use FBOConfig to add their desired FBOs to FBM Aug 16, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 16, 2016

Member

Uh oh, something went wrong with the build. Need to check on that

Member

GooeyHub commented Aug 16, 2016

Uh oh, something went wrong with the build. Need to check on that

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 16, 2016

Member

Uh oh, something went wrong with the build. Need to check on that

Member

GooeyHub commented Aug 16, 2016

Uh oh, something went wrong with the build. Need to check on that

@@ -49,6 +57,9 @@ public StateChange getDefaultInstance() {
@Override
public RenderPipelineTask generateTask() {
if (task == null) {
if (isTheDefaultInstance()) {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

if (this.isTheDefaultInstance...

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

if (this.isTheDefaultInstance...

This comment has been minimized.

@tdgunes

tdgunes Aug 18, 2016

Member

Sorry, but I think this is already done in the other PR.

@tdgunes

tdgunes Aug 18, 2016

Member

Sorry, but I think this is already done in the other PR.

* @param fboName the urn of an FBO
* @return True if an FBO associated with the given name exists. False otherwise.
*/
public boolean bindFboColorTexture(ResourceUrn fboName) {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Let's add:

TODO: remove bindFbo*Texture methods from this class and encapsulate the associated methods in the FBO class within StateChange implementations.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Let's add:

TODO: remove bindFbo*Texture methods from this class and encapsulate the associated methods in the FBO class within StateChange implementations.

This comment has been minimized.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Just saw the note in the interface. Never mind.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Just saw the note in the interface. Never mind.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 18, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 18, 2016

Hooray Jenkins reported success with all tests good!

/**
* TODO: Add javadocs
*/
public enum DefaultDynamicFBOs {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Please remind me. What's the usefulness of having them as enums rather than standard FBOs instantiated when the DisplayResolutionDependentFBOs gets instantiated or initialized?

@emanuele3d

emanuele3d Aug 18, 2016

Contributor

Please remind me. What's the usefulness of having them as enums rather than standard FBOs instantiated when the DisplayResolutionDependentFBOs gets instantiated or initialized?

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 18, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 18, 2016

Hooray Jenkins reported success with all tests good!

private float currentExposure;
private boolean isTakingScreenshot;

This comment has been minimized.

@emanuele3d

emanuele3d Aug 19, 2016

Contributor

Very tempted of suggesting to rename this to "isGrabbingTheScreen".

@emanuele3d

emanuele3d Aug 19, 2016

Contributor

Very tempted of suggesting to rename this to "isGrabbingTheScreen".

This comment has been minimized.

@tdgunes

tdgunes Aug 19, 2016

Member

Using "grab" in other methods was not very appropriate. Sounded a little weird. Let's keep it for the time being.

@tdgunes

tdgunes Aug 19, 2016

Member

Using "grab" in other methods was not very appropriate. Sounded a little weird. Let's keep it for the time being.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 19, 2016

Hooray Jenkins reported success with all tests good!

if (renderingConfig.isDynamicShadows()) {
int shadowMapResFromSettings = (int) evt.getNewValue();
shadowMapResolution = new FBO.Dimensions(shadowMapResFromSettings, shadowMapResFromSettings);

This comment has been minimized.

@emanuele3d

emanuele3d Aug 19, 2016

Contributor

Let's remove the unnecessary empty line here.

@emanuele3d

emanuele3d Aug 19, 2016

Contributor

Let's remove the unnecessary empty line here.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 19, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 19, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 19, 2016

Contributor

180 comments (with this one). I wonder if that's a record for Terasology. 😄

Anyway, there are a few comments that did not get acknowledged. @tdgunes, you might want to go through them just to make sure you did everything but I think we could be in merge-ready territory already.

Can you please fiddle with the video settings please?

  • disabling/enabling individual effects and combinations
  • changing video resolution and resizing the display in window mode
  • checking the debug displays for artifacts

Just to make sure everything works as expected.

Contributor

emanuele3d commented Aug 19, 2016

180 comments (with this one). I wonder if that's a record for Terasology. 😄

Anyway, there are a few comments that did not get acknowledged. @tdgunes, you might want to go through them just to make sure you did everything but I think we could be in merge-ready territory already.

Can you please fiddle with the video settings please?

  • disabling/enabling individual effects and combinations
  • changing video resolution and resizing the display in window mode
  • checking the debug displays for artifacts

Just to make sure everything works as expected.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 19, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Aug 19, 2016

Member

@emanuele3d It took a while to go through all of the comments again (for the third time, hoping not missed a one). I tested on various configuration, also checked debug displays and changing video resolution in window mode. I think it can be now tagged as [Ready For Merge].

Member

tdgunes commented Aug 19, 2016

@emanuele3d It took a while to go through all of the comments again (for the third time, hoping not missed a one). I tested on various configuration, also checked debug displays and changing video resolution in window mode. I think it can be now tagged as [Ready For Merge].

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 19, 2016

Contributor

@tdgunes, I have some bad news.

You need to kiss goodbye to this PR as from my perspective it's time to merge it.

😆

Congrats!!!

Contributor

emanuele3d commented Aug 19, 2016

@tdgunes, I have some bad news.

You need to kiss goodbye to this PR as from my perspective it's time to merge it.

😆

Congrats!!!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 19, 2016

Member

Cursory testing shows no obvious issues - merging happily! Thanks :-)

Member

Cervator commented Aug 19, 2016

Cursory testing shows no obvious issues - merging happily! Thanks :-)

@Cervator Cervator merged commit 43e7aea into MovingBlocks:develop Aug 19, 2016

1 check passed

default Build finished.
Details

Cervator added a commit that referenced this pull request Aug 19, 2016

@Cervator Cervator removed the in progress label Aug 19, 2016

@Cervator Cervator added this to the Alpha 4 milestone Aug 19, 2016

@tdgunes tdgunes changed the title from [WIP] Nodes use FBOConfig to add their desired FBOs to FBM to Nodes use FBOConfig to add their desired FBOs to FBM Aug 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment