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

New StateChanges for DAG #2427

Merged
merged 32 commits into from Aug 21, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Jul 31, 2016

Important note for reviewers: This PR is based on #2425, please not merge after that one is merged.

Contains

As title mentions, adds five new state changes for All obvious State Changes implemented

How to test

Please check any weirdness in the log and rendering of reflections, hdr and shadows.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 31, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 31, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from New StateChanges for DAG to [RFR] New StateChanges for DAG Jul 31, 2016

@@ -539,6 +539,8 @@ private boolean isLightVisible(Vector3f positionViewSpace, LightComponent compon
@Override
public boolean isHeadUnderWater() {
// TODO: Making this as a subscribable value especially for node "ChunksRefractiveReflectiveNode",
// TODO: glDisable and glEnable state changes on that node will be dynamically added/removed based on this value.

This comment has been minimized.

@emanuele3d

emanuele3d Aug 1, 2016

Contributor

Hmmmm... careful here. Adding/removing nodes will probably be a relatively expensive operation. The head going under/above water might happen multiple times in a short period. So, we'll have to think carefully about this. Perhaps rephrase this as a question: // TODO: review - what happens RenderGraph-wise when the head goes under water?

@emanuele3d

emanuele3d Aug 1, 2016

Contributor

Hmmmm... careful here. Adding/removing nodes will probably be a relatively expensive operation. The head going under/above water might happen multiple times in a short period. So, we'll have to think carefully about this. Perhaps rephrase this as a question: // TODO: review - what happens RenderGraph-wise when the head goes under water?

This comment has been minimized.

@tdgunes

tdgunes Aug 3, 2016

Member

Hmm, well what I had in mind was not adding/removing nodes whenever player dives into to the water. I thought what if for instance ChunksRefractiveReflectiveNode is informed about this event and state changes added or removed according to this event.

@tdgunes

tdgunes Aug 3, 2016

Member

Hmm, well what I had in mind was not adding/removing nodes whenever player dives into to the water. I thought what if for instance ChunksRefractiveReflectiveNode is informed about this event and state changes added or removed according to this event.

This comment has been minimized.

@emanuele3d

emanuele3d Aug 3, 2016

Contributor

Sorry, I misread the comment. I now understand you were referring to adding/removing StateChange instances. We can try. Perhaps it won't be much of a performance hit. Certainly we should try to have no instantiation occurring during the switches. I.e. have everything ready at initialization and then only change the content of desiredStateChanges set.

That been said changes in the set at this stage will always trigger a refresh of the whole intermediate and task lists. I'd be surprised if this doesn't have any impact. For this reason I suspect that whatever happens when the head is underwater might have to be handled with an if block within a Node.process() method, just to keep everything very responsive.

@emanuele3d

emanuele3d Aug 3, 2016

Contributor

Sorry, I misread the comment. I now understand you were referring to adding/removing StateChange instances. We can try. Perhaps it won't be much of a performance hit. Certainly we should try to have no instantiation occurring during the switches. I.e. have everything ready at initialization and then only change the content of desiredStateChanges set.

That been said changes in the set at this stage will always trigger a refresh of the whole intermediate and task lists. I'd be surprised if this doesn't have any impact. For this reason I suspect that whatever happens when the head is underwater might have to be handled with an if block within a Node.process() method, just to keep everything very responsive.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 6, 2016

Contributor

@tdgunes: what's happening with this PR? I'd really like to see this one closed before the FBO one proceeds. Furthermore, I suspect we need to make a pause here and assess what's happening with the issue #2418. Perhaps if this PR is completed and all nodes take advantage of the new state changes that issue might get solved automagically. But I wouldn't bet on it.

Contributor

emanuele3d commented Aug 6, 2016

@tdgunes: what's happening with this PR? I'd really like to see this one closed before the FBO one proceeds. Furthermore, I suspect we need to make a pause here and assess what's happening with the issue #2418. Perhaps if this PR is completed and all nodes take advantage of the new state changes that issue might get solved automagically. But I wouldn't bet on it.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 8, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 8, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 8, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 8, 2016

Hooray Jenkins reported success with all tests good!

task.setDimensions(fbo.width(), fbo.height());
} else {
task.setDimensions(defaultDynamicFBO.width(), defaultDynamicFBO.height());
}

This comment has been minimized.

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

Hmmm. I'm really not liking how the default FBOs need to have special handling. For me they should be pretty normal FBOs that nodes can rely on are available right from the start. Then, ideally, they should be easily reachable, i.e. through static variables. Can you write a TODO in the default FBOs class on this regard?

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

Hmmm. I'm really not liking how the default FBOs need to have special handling. For me they should be pretty normal FBOs that nodes can rely on are available right from the start. Then, ideally, they should be easily reachable, i.e. through static variables. Can you write a TODO in the default FBOs class on this regard?

/**
* TODO: Add javadocs
*/
public abstract class ConditionDependentNode extends AbstractNode implements PropertyChangeListener {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

This is a tricky class I find. I just understood that it is written this way to allow for multiple conditions. I'm not sure if we have a use case for that already but... ok. I guess the one problem I have is with the term "Condition". It's quite generic while the class really only deals with boolean values. Not only that: all conditions have to be true. So, a node that, say, needs to be inserted into the graph if the condition is false (for whatever reason) can't use this.

So, it seems to me that the usability of this class is quite narrow. I'd remove it in favor of nodes subscribing to the rendering config on initialization and implementing the PropertyChangeListener interface, to leave maximum freedom to the node developers.

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

This is a tricky class I find. I just understood that it is written this way to allow for multiple conditions. I'm not sure if we have a use case for that already but... ok. I guess the one problem I have is with the term "Condition". It's quite generic while the class really only deals with boolean values. Not only that: all conditions have to be true. So, a node that, say, needs to be inserted into the graph if the condition is false (for whatever reason) can't use this.

So, it seems to me that the usability of this class is quite narrow. I'd remove it in favor of nodes subscribing to the rendering config on initialization and implementing the PropertyChangeListener interface, to leave maximum freedom to the node developers.

This comment has been minimized.

@tdgunes

tdgunes Aug 21, 2016

Member

Module developers can already do what you are saying. Freedom is preserved! 😄 This class actually introduces an helper method requiresCondition which accepts a closure(lambda function). This function can be anything, a very good use case for this one is in BlurPassesNode.

    private FBO sceneToneMapped;

    @Override
    public void initialise() {
        renderingConfig = config.getRendering();
        blur = worldRenderer.getMaterial("engine:prog.blur");

        renderingConfig.subscribe(RenderingConfig.BLUR_INTENSITY, this);
        requiresCondition(() -> renderingConfig.getBlurIntensity() != 0);
        requiresFBO(new FBOConfig(BLUR_0, HALF_SCALE, FBO.Type.DEFAULT), displayResolutionDependentFBOs);
        requiresFBO(new FBOConfig(BLUR_1, HALF_SCALE, FBO.Type.DEFAULT), displayResolutionDependentFBOs);
        requiresFBO(new FBOConfig(TONE_MAPPED, FULL_SCALE, FBO.Type.HDR), displayResolutionDependentFBOs);
    }

I was between the term ConditionDependentNode and ConfigDependentNode. Condition seemed more generic, as you mentioned. Another benefit of having this class is reducing huge amount of redundant code. (Same lines in many different nodes.) Therefore, for the time being, I would not remove this class. In long term goals, I would investigate better ways to connect current configurations with nodes.

@tdgunes

tdgunes Aug 21, 2016

Member

Module developers can already do what you are saying. Freedom is preserved! 😄 This class actually introduces an helper method requiresCondition which accepts a closure(lambda function). This function can be anything, a very good use case for this one is in BlurPassesNode.

    private FBO sceneToneMapped;

    @Override
    public void initialise() {
        renderingConfig = config.getRendering();
        blur = worldRenderer.getMaterial("engine:prog.blur");

        renderingConfig.subscribe(RenderingConfig.BLUR_INTENSITY, this);
        requiresCondition(() -> renderingConfig.getBlurIntensity() != 0);
        requiresFBO(new FBOConfig(BLUR_0, HALF_SCALE, FBO.Type.DEFAULT), displayResolutionDependentFBOs);
        requiresFBO(new FBOConfig(BLUR_1, HALF_SCALE, FBO.Type.DEFAULT), displayResolutionDependentFBOs);
        requiresFBO(new FBOConfig(TONE_MAPPED, FULL_SCALE, FBO.Type.HDR), displayResolutionDependentFBOs);
    }

I was between the term ConditionDependentNode and ConfigDependentNode. Condition seemed more generic, as you mentioned. Another benefit of having this class is reducing huge amount of redundant code. (Same lines in many different nodes.) Therefore, for the time being, I would not remove this class. In long term goals, I would investigate better ways to connect current configurations with nodes.

@@ -131,5 +132,6 @@ public void swapReadWriteBuffers() {
WRITE_ONLY_GBUFFER.setFbo(fbo);
fboLookup.put(READ_ONLY_GBUFFER.getName(), READ_ONLY_GBUFFER.getFbo());
fboLookup.put(WRITE_ONLY_GBUFFER.getName(), WRITE_ONLY_GBUFFER.getFbo());
notifySubscribers();

This comment has been minimized.

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

Looking at this line I noticed that in update() subscribers are notified in createFBOs and before updateDefaultFBOs() is called. If I am correct you might want to move the notifySubscribers() line from createFBOs to the end of the IF block in update(), just after the line updateDefaultFBOs().

@emanuele3d

emanuele3d Aug 20, 2016

Contributor

Looking at this line I noticed that in update() subscribers are notified in createFBOs and before updateDefaultFBOs() is called. If I am correct you might want to move the notifySubscribers() line from createFBOs to the end of the IF block in update(), just after the line updateDefaultFBOs().

This comment has been minimized.

@tdgunes

tdgunes Aug 21, 2016

Member

Yes, true. But since DefaultFBOs internal FBO reference is up to date just before task list is executed, this didn't cause any harm. On the other hand, there can be some unexpected bugs that are not currently found because of this. So I think it's better to move notifySubscribers after updateDefaultFBOs().

@tdgunes

tdgunes Aug 21, 2016

Member

Yes, true. But since DefaultFBOs internal FBO reference is up to date just before task list is executed, this didn't cause any harm. On the other hand, there can be some unexpected bugs that are not currently found because of this. So I think it's better to move notifySubscribers after updateDefaultFBOs().

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 20, 2016

Contributor

@tdgunes: I finished the review. Looks good! Relatively few issues to deal with, maybe a couple of issues a little more involved but nothing horribly wrong. And I like how the Node.process() methods are now very, very lean in most cases.

Contributor

emanuele3d commented Aug 20, 2016

@tdgunes: I finished the review. Looks good! Relatively few issues to deal with, maybe a couple of issues a little more involved but nothing horribly wrong. And I like how the Node.process() methods are now very, very lean in most cases.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 21, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 21, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 21, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 21, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 21, 2016

Contributor

Declaring this PR officially GOOD TO MERGE!!! Yieppieeee!!

Contributor

emanuele3d commented Aug 21, 2016

Declaring this PR officially GOOD TO MERGE!!! Yieppieeee!!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 21, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 21, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from [RFR] New StateChanges for DAG to [Good To Merge] New StateChanges for DAG Aug 21, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 21, 2016

Member

Tested again and no obvious issues when just varying shadows and such under normal conditions, water reflections on global still look awesome but - I think there may be an issue (at least for me) with lighting, specifically when the player equips a luminous object like a torch.

Last time I tested the "underside" view of the world changed dramatically when I equipped a torch which I wrote off as being an oddity from being under the world, but I just realized it happens when just walking around on the right side of the world and switching to a torch. The sky goes black, water disappears, and some weird skysphere-related pattern becomes visible.

While I have nothing to base this on I suspect this is probably something simple for a 3d wizard (similar effects have happened before). It appears active across all settings I tried, from minimal to ultra, before or after changing settings in-game. But it would be good to know if anybody else sees it :-)

Black sky:

blacksky

No Water:

nowater

Weird skysphere pattern or something:

skyspherepattern

Double checked that it seems to be caused by this PR, at least when merged on top of latest develop.

Member

Cervator commented Aug 21, 2016

Tested again and no obvious issues when just varying shadows and such under normal conditions, water reflections on global still look awesome but - I think there may be an issue (at least for me) with lighting, specifically when the player equips a luminous object like a torch.

Last time I tested the "underside" view of the world changed dramatically when I equipped a torch which I wrote off as being an oddity from being under the world, but I just realized it happens when just walking around on the right side of the world and switching to a torch. The sky goes black, water disappears, and some weird skysphere-related pattern becomes visible.

While I have nothing to base this on I suspect this is probably something simple for a 3d wizard (similar effects have happened before). It appears active across all settings I tried, from minimal to ultra, before or after changing settings in-game. But it would be good to know if anybody else sees it :-)

Black sky:

blacksky

No Water:

nowater

Weird skysphere pattern or something:

skyspherepattern

Double checked that it seems to be caused by this PR, at least when merged on top of latest develop.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 21, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 21, 2016

Hooray Jenkins reported success with all tests good!

@Cervator Cervator merged commit 45735b4 into MovingBlocks:develop Aug 21, 2016

1 check passed

default Build finished.
Details

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

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

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 21, 2016

Member

Merged! Discussed briefly with @tdgunes on Slack and the one problematic Node was reverted for the time being, seems there's something quirky about the original. No surprise there - it'll take time to unravel all the spiderwebs that built up over the years :-)

Thanks!

Member

Cervator commented Aug 21, 2016

Merged! Discussed briefly with @tdgunes on Slack and the one problematic Node was reverted for the time being, seems there's something quirky about the original. No surprise there - it'll take time to unravel all the spiderwebs that built up over the years :-)

Thanks!

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

@tdgunes tdgunes changed the title from [Good To Merge] New StateChanges for DAG to New StateChanges for DAG Aug 21, 2016

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