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

Architecture for Redundant State Elimination #2399

Merged
merged 18 commits into from Jul 26, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Jul 8, 2016

Contains

Initial architecture for redundant state elimination, render task creation and so on.

How to test

Play the game with different video settings and observe any irregularities.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 8, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 8, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from [WIP] Architecture for Redundant State Elimination to [NFM] Architecture for Redundant State Elimination Jul 8, 2016

node.initialise();
return type.cast(node);
}
}

This comment has been minimized.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

Interesting that github shows this. Does it have consequences? A quick look on StackOverflow seems to hint that this is a relic from the past but tools will flag it up as an issue, which is per se a good reason to end a file with a newline character.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

Interesting that github shows this. Does it have consequences? A quick look on StackOverflow seems to hint that this is a relic from the past but tools will flag it up as an issue, which is per se a good reason to end a file with a newline character.

This comment has been minimized.

@Cervator

Cervator Jul 8, 2016

Member

The little red thing? I see it on occasion, I suspect it happens when new files are created on different OSes, causing the difference in line/file endings, although Git is supposed to and I guess does handle it for line endings. But maybe it doesn't catch the very final line in some cases..

Some user actions can impact it and I think we could explicitly make sure we always have a blank line at the end of everything. I don't think it really causes issues but it is a fair practice to follow, just easy to miss. I'm not sure we have (or can have?) a Checkstyle guard against it, so we often don't notice till the GitHub diff flags it like this.

@Cervator

Cervator Jul 8, 2016

Member

The little red thing? I see it on occasion, I suspect it happens when new files are created on different OSes, causing the difference in line/file endings, although Git is supposed to and I guess does handle it for line endings. But maybe it doesn't catch the very final line in some cases..

Some user actions can impact it and I think we could explicitly make sure we always have a blank line at the end of everything. I don't think it really causes issues but it is a fair practice to follow, just easy to miss. I'm not sure we have (or can have?) a Checkstyle guard against it, so we often don't notice till the GitHub diff flags it like this.

This comment has been minimized.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

Hehe, for me the fact that you don't get the little warning here in github is enough of a reason to automatically add a newline at the end of the file if there isn't one. =)

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

Hehe, for me the fact that you don't get the little warning here in github is enough of a reason to automatically add a newline at the end of the file if there isn't one. =)

This comment has been minimized.

@tdgunes

tdgunes Jul 10, 2016

Member

Seems like I missed checkstyle's warning, thought that I fixed that one. It will be resolved in the next commit. 😄

@tdgunes

tdgunes Jul 10, 2016

Member

Seems like I missed checkstyle's warning, thought that I fixed that one. It will be resolved in the next commit. 😄

public <T extends Node> T createInstance(Class<T> type) {
// Attempt constructor-based injection first
T node = InjectionHelper.createWithConstructorInjection(type, context);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

I'm puzzled by the comment. If this is an "attempt"... it can be successful or it can fail. Where is the code to handle failure?

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

I'm puzzled by the comment. If this is an "attempt"... it can be successful or it can fail. Where is the code to handle failure?

This comment has been minimized.

@tdgunes

tdgunes Jul 9, 2016

Member

Actually, that part of the node creation and comments were introduced by @msteiger in his PR in my fork, tdgunes@fba3ccf. After merging, I left it as is. I just moved it to NodeFactory class.

@tdgunes

tdgunes Jul 9, 2016

Member

Actually, that part of the node creation and comments were introduced by @msteiger in his PR in my fork, tdgunes@fba3ccf. After merging, I left it as is. I just moved it to NodeFactory class.

This comment has been minimized.

@emanuele3d

emanuele3d Jul 9, 2016

Contributor

Thank you @tdgunes, let's ask @msteiger then. Martin? Can that piece of code fail?

@emanuele3d

emanuele3d Jul 9, 2016

Contributor

Thank you @tdgunes, let's ask @msteiger then. Martin? Can that piece of code fail?

This comment has been minimized.

@msteiger

msteiger Jul 12, 2016

Member

That particular method will try to inject fields through parameters, but fall back to using the default constructor.
Still, it can and it is not even unlikely (e.g. no default constructor).
That said, I suggest calling the entire method in a try-catch block. It could be enforced by declaring an (runtime) exception that this method could throw or adding some javadoc.

@msteiger

msteiger Jul 12, 2016

Member

That particular method will try to inject fields through parameters, but fall back to using the default constructor.
Still, it can and it is not even unlikely (e.g. no default constructor).
That said, I suggest calling the entire method in a try-catch block. It could be enforced by declaring an (runtime) exception that this method could throw or adding some javadoc.

This comment has been minimized.

@tdgunes

tdgunes Jul 12, 2016

Member

@msteiger It seems like that particular method, internally returns an optional value(.get() returns null) and logs the failure of the attempt. How about:

    public <T extends Node> T createInstance(Class<T> type) {
        // Attempt constructor-based injection first
        T node = InjectionHelper.createWithConstructorInjection(type, context);
        Preconditions.checkNotNull(node, "Constructor-based injection attempt Failed to initialise the node.");
        // Then fill @In fields
        InjectionHelper.inject(node, context);
        node.initialise();
        return type.cast(node);
    }

as we previously did in DebugMetricsSystem.

@tdgunes

tdgunes Jul 12, 2016

Member

@msteiger It seems like that particular method, internally returns an optional value(.get() returns null) and logs the failure of the attempt. How about:

    public <T extends Node> T createInstance(Class<T> type) {
        // Attempt constructor-based injection first
        T node = InjectionHelper.createWithConstructorInjection(type, context);
        Preconditions.checkNotNull(node, "Constructor-based injection attempt Failed to initialise the node.");
        // Then fill @In fields
        InjectionHelper.inject(node, context);
        node.initialise();
        return type.cast(node);
    }

as we previously did in DebugMetricsSystem.

@@ -13,13 +13,14 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.terasology.rendering.dag;

This comment has been minimized.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

I'm thinking, if everything is going into a "nodes" package it might be appropriate to remove the word "Node" from the name of every node class.

By the way, this reminds me of a trick to keep PRs "cleaner". When you do a trivial but necessary change, like moving a bunch of classes into a sub-package, wrap that into its own commit and make sure the reviewers know that commit is trivial. That way the reviewers know to focus on the following commit(s) and have far fewer changes to go through.

Look for example at my #1745 PR: each commit has a specific purpose, the idea being reviewers have the chance to focus on commits that are doing serious changes rather than simple after-change maintenance.

@emanuele3d

emanuele3d Jul 8, 2016

Contributor

I'm thinking, if everything is going into a "nodes" package it might be appropriate to remove the word "Node" from the name of every node class.

By the way, this reminds me of a trick to keep PRs "cleaner". When you do a trivial but necessary change, like moving a bunch of classes into a sub-package, wrap that into its own commit and make sure the reviewers know that commit is trivial. That way the reviewers know to focus on the following commit(s) and have far fewer changes to go through.

Look for example at my #1745 PR: each commit has a specific purpose, the idea being reviewers have the chance to focus on commits that are doing serious changes rather than simple after-change maintenance.

This comment has been minimized.

@tdgunes

tdgunes Jul 9, 2016

Member

I am fond of shorter, simple and clean names. But I am not sure about how module developers interact with the existing nodes. So can we decide this in future? (I can a put a comment to Node interface for remembering this.)

Thanks for the making PRs "cleaner" advice. I will keep this in mind. I thought "reviewers" usually just check whole diff and give feedback accordingly, not going throughout every individual commit.

@tdgunes

tdgunes Jul 9, 2016

Member

I am fond of shorter, simple and clean names. But I am not sure about how module developers interact with the existing nodes. So can we decide this in future? (I can a put a comment to Node interface for remembering this.)

Thanks for the making PRs "cleaner" advice. I will keep this in mind. I thought "reviewers" usually just check whole diff and give feedback accordingly, not going throughout every individual commit.

This comment has been minimized.

@emanuele3d

emanuele3d Jul 9, 2016

Contributor

On your first paragraph: sure. Let's add

//TODO: consider removing the word "Node" from the name of all Node implementations now that they are in the dag.nodes package.

to the Node interface.

On your second paragraph: indeed it doesn't make sense to go through individual commits unless the individual commits have been "designed" and documented to be reviewed separately or skipped altogether. =)

@emanuele3d

emanuele3d Jul 9, 2016

Contributor

On your first paragraph: sure. Let's add

//TODO: consider removing the word "Node" from the name of all Node implementations now that they are in the dag.nodes package.

to the Node interface.

On your second paragraph: indeed it doesn't make sense to go through individual commits unless the individual commits have been "designed" and documented to be reviewed separately or skipped altogether. =)

This comment has been minimized.

@tdgunes

tdgunes Jul 9, 2016

Member

Agreed, will do!

@tdgunes

tdgunes Jul 9, 2016

Member

Agreed, will do!

@tdgunes tdgunes changed the title from [NFM] Architecture for Redundant State Elimination to [WIP] Architecture for Redundant State Elimination Jul 11, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 11, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 11, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from [WIP] Architecture for Redundant State Elimination to [RFM] Architecture for Redundant State Elimination Jul 12, 2016

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jul 12, 2016

Member

@emanuele3d and @msteiger my latest commit is ready for merge. (Sorry for the huge delay, I took a while). I re-checked it again if I missed anything according to the feedback, (hoping that I am not). Before reviewing that, I wanted to mention couple of important things:

  • Since overriding static methods seems not feasible within interfaces in Java, my latest commit introduces a simple Storage for the default values called DefaultStateChangeStorage. As mentioned in the comments, it will be maintainers duty to not miss putting default value of the implementation of StateChange inside the storage. (As in stateChangeTaskMap.)
  • RenderPipelineOptimizer is not doing any optimizations in this PR. But it's ready for defining state changes in nodes with the help of stateChangeTaskMap and defaultStateChangeStorage.
  • I changed the title to "Ready for Merge", even though it's an abstract architectural change. Because at this point PR does not break anything and also it currently works without any noticeable problem.

Edit: I've just added Preconditions check to the NodeFactory as a local change. If @msteiger approves, I will push that change and hopefully we can merge it! 🎊 🎉

Member

tdgunes commented Jul 12, 2016

@emanuele3d and @msteiger my latest commit is ready for merge. (Sorry for the huge delay, I took a while). I re-checked it again if I missed anything according to the feedback, (hoping that I am not). Before reviewing that, I wanted to mention couple of important things:

  • Since overriding static methods seems not feasible within interfaces in Java, my latest commit introduces a simple Storage for the default values called DefaultStateChangeStorage. As mentioned in the comments, it will be maintainers duty to not miss putting default value of the implementation of StateChange inside the storage. (As in stateChangeTaskMap.)
  • RenderPipelineOptimizer is not doing any optimizations in this PR. But it's ready for defining state changes in nodes with the help of stateChangeTaskMap and defaultStateChangeStorage.
  • I changed the title to "Ready for Merge", even though it's an abstract architectural change. Because at this point PR does not break anything and also it currently works without any noticeable problem.

Edit: I've just added Preconditions check to the NodeFactory as a local change. If @msteiger approves, I will push that change and hopefully we can merge it! 🎊 🎉

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 12, 2016

Contributor

I haven't seen the changes yet, but I would suggest this PR is ready for review, not for merge.

Contributor

emanuele3d commented Jul 12, 2016

I haven't seen the changes yet, but I would suggest this PR is ready for review, not for merge.

@tdgunes tdgunes changed the title from [RFM] Architecture for Redundant State Elimination to [RFR] Architecture for Redundant State Elimination Jul 12, 2016

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jul 12, 2016

Member

@emanuele3d: My rationale behind tagging it as RFM, I believe PR is in a merge-able state. But of course my intention is awaiting your feedback, prior to the merge. Let me make it "RFR" for the time being.

Member

tdgunes commented Jul 12, 2016

@emanuele3d: My rationale behind tagging it as RFM, I believe PR is in a merge-able state. But of course my intention is awaiting your feedback, prior to the merge. Let me make it "RFR" for the time being.

tdgunes added some commits Jul 8, 2016

Remove AbstractStateChange, remove generics from StateChange, add sub…
…scribe & unsubscribe methods. Add toString for logging purposes.
Add toString() for logging purposes, core functionality to RenderTask…
…ListGenerator, BindFBO desired state changes to shadowMapNode and WorldReflectionNode
Add @manu3d's version of generateTaskList(), tune tests (also a new c…
…ase), move necessary inner helper classes to their files
@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jul 25, 2016

Member

@emanuele3d and @Cervator, I rebased this onto latest changes in develop branch. (Resolved conflicts!) I am very happy to tag this PR as [Ready for Merge].

Member

tdgunes commented Jul 25, 2016

@emanuele3d and @Cervator, I rebased this onto latest changes in develop branch. (Resolved conflicts!) I am very happy to tag this PR as [Ready for Merge].

@tdgunes tdgunes changed the title from [NFM] Architecture for Redundant State Elimination to [RFR] Architecture for Redundant State Elimination Jul 25, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 25, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 25, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 25, 2016

Contributor

I just wanted to confirm this PR for me is ready for merge.

Contributor

emanuele3d commented Jul 25, 2016

I just wanted to confirm this PR for me is ready for merge.

@Cervator Cervator merged commit 6e3acce into develop Jul 26, 2016

1 check passed

default Build finished.
Details

@Cervator Cervator removed the in progress label Jul 26, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jul 26, 2016

Member

Awesome. Thanks again both for great work :-)

While unrelated to this PR I discovered #2418 during testing. Otherwise I didn't notice anything out of the ordinary.

Yay progress!

Member

Cervator commented Jul 26, 2016

Awesome. Thanks again both for great work :-)

While unrelated to this PR I discovered #2418 during testing. Otherwise I didn't notice anything out of the ordinary.

Yay progress!

@Cervator Cervator added this to the Alpha 3 milestone Jul 26, 2016

@tdgunes tdgunes changed the title from [RFR] Architecture for Redundant State Elimination to Architecture for Redundant State Elimination Jul 26, 2016

@Cervator Cervator deleted the dag-pipeline branch Aug 1, 2016

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