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

[RFR] Introduces API for module developers to add graphical tweaks to their mods #2449

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
7 participants
@tdgunes
Member

tdgunes commented Aug 22, 2016

Contains

Enables module developers to add their own nodes/render graph to rendering engine.

How to test

Try to enable the tutorial module and run the game. Confirm that colours are inverted.

Outstanding before merging

Which parts to expose to module developer is a good start. Because of the deadline, I might have wrongly put @API annotation to somer parts, that we don't want to. Better to discuss this with @msteiger and @emanuele3d .

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Aug 22, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Aug 22, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 22, 2016

Member

Can confirm that the tutorial module works! :-)

I talked to @tdgunes some on Slack about potential architecture approaches to let modules affect rendering like this. I suggested a temporary approach that's less than perfect just so we can see it in action, and heck I'm even getting in on the action by writing my own shader based on this tutorial/API! Which is certainly a thing I had never imagined doing :D

The more ideal approach would probably hide some boilerplate code behind a new annotation like @RegisterNode you would apply to a System in a module. And/Or maybe a new interface for systems that would require a method that would add/remove suitable nodes. That all could help handle any boilerplate stuff needed to hook into the rendering system leaving the user to just apply desired changes. Right now the module does a bit too much just to make it work

I recommend we review and merge this with the understanding that the permanent approach along with some review of what modules should or shouldn't have access to later. We should submit a separate issue that could be done during or after GSOC, but before the next game release.

Any small stuff that could be improved in the PR immediately is fair stuff (including any @API stuff not needed), I can't speak to any of the details just warrant that they appear to work! :-)

Edit: After understanding this a little better (actually wrote my own shader woo! Totally didn't just copy paste and edit numbers ...) I expect we are probably more looking at a @RegisterNode to apply to Node objects that in turn get hooked in without having to write a SampleGraphicsSystem like in the tutorial module at the moment. Probably there are fancier 3d effects that might still need a full system, but my goofy little shader + node could probably be hooked up with 3-4 small files and an annotation :-)

Member

Cervator commented Aug 22, 2016

Can confirm that the tutorial module works! :-)

I talked to @tdgunes some on Slack about potential architecture approaches to let modules affect rendering like this. I suggested a temporary approach that's less than perfect just so we can see it in action, and heck I'm even getting in on the action by writing my own shader based on this tutorial/API! Which is certainly a thing I had never imagined doing :D

The more ideal approach would probably hide some boilerplate code behind a new annotation like @RegisterNode you would apply to a System in a module. And/Or maybe a new interface for systems that would require a method that would add/remove suitable nodes. That all could help handle any boilerplate stuff needed to hook into the rendering system leaving the user to just apply desired changes. Right now the module does a bit too much just to make it work

I recommend we review and merge this with the understanding that the permanent approach along with some review of what modules should or shouldn't have access to later. We should submit a separate issue that could be done during or after GSOC, but before the next game release.

Any small stuff that could be improved in the PR immediately is fair stuff (including any @API stuff not needed), I can't speak to any of the details just warrant that they appear to work! :-)

Edit: After understanding this a little better (actually wrote my own shader woo! Totally didn't just copy paste and edit numbers ...) I expect we are probably more looking at a @RegisterNode to apply to Node objects that in turn get hooked in without having to write a SampleGraphicsSystem like in the tutorial module at the moment. Probably there are fancier 3d effects that might still need a full system, but my goofy little shader + node could probably be hooked up with 3-4 small files and an annotation :-)

@In
private ShaderManager shaderManager;
public void setRenderGraph(RenderGraph renderGraph) {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 22, 2016

Contributor

At least let's add some javadoc placeholders!!!

@emanuele3d

emanuele3d Aug 22, 2016

Contributor

At least let's add some javadoc placeholders!!!

@@ -180,6 +180,17 @@ private GLSLMaterial prepareAndStoreShaderProgramInstance(String title, ShaderPa
return material;
}
@Override
public void loadShader(ResourceUrn shaderName, ShaderParameters shaderParameters) {

This comment has been minimized.

@emanuele3d

emanuele3d Aug 22, 2016

Contributor

I think we should eliminate the code duplication between this method and prepareAndStoreShaderProgramInstance(String, ShaderParameters).

I think this can be achieved by placing the common code in a separate private method "_loadShader" or "internalLoadShader". Then, loadShader only checks that the namespace in the urn is NOT "engine:". Meanwhile prepareAndStoreShaderProgramInstance just adds the "engine:" part to the input string. Both methods than call "_loadShader" to do the actual shader loading.

@emanuele3d

emanuele3d Aug 22, 2016

Contributor

I think we should eliminate the code duplication between this method and prepareAndStoreShaderProgramInstance(String, ShaderParameters).

I think this can be achieved by placing the common code in a separate private method "_loadShader" or "internalLoadShader". Then, loadShader only checks that the namespace in the urn is NOT "engine:". Meanwhile prepareAndStoreShaderProgramInstance just adds the "engine:" part to the input string. Both methods than call "_loadShader" to do the actual shader loading.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 22, 2016

Contributor

I can see how, due to the GSOC deadline, this is going to get into develop no matter my comments, but it's really the wrong approach and it will unnecessarily pollute the branch. We might want to consider this a learning experience, where during GSOC projects we'll want the last PR to be submitted some days before the deadline, so that we do not risk rushing it through for the benefit of the student and at the cost of the repository, leaving time for a proper wrap up period.

Right now @tdgunes you are clearing a 24 items list to re-insert a straight copy of it with an additional node. This also cascaded into having to open functionality to set the shadow map node, set the whole render graph and clear all FBOs.

How about removing all that and simply adding methods to append, prepend and/or insert a node into the node list? Wouldn't that make things simpler? If anything SampleGraphicsSystem.initialise()?

Really, it seems to me sleep deprivation and caffeine are are taking their toll!!! 😞

Contributor

emanuele3d commented Aug 22, 2016

I can see how, due to the GSOC deadline, this is going to get into develop no matter my comments, but it's really the wrong approach and it will unnecessarily pollute the branch. We might want to consider this a learning experience, where during GSOC projects we'll want the last PR to be submitted some days before the deadline, so that we do not risk rushing it through for the benefit of the student and at the cost of the repository, leaving time for a proper wrap up period.

Right now @tdgunes you are clearing a 24 items list to re-insert a straight copy of it with an additional node. This also cascaded into having to open functionality to set the shadow map node, set the whole render graph and clear all FBOs.

How about removing all that and simply adding methods to append, prepend and/or insert a node into the node list? Wouldn't that make things simpler? If anything SampleGraphicsSystem.initialise()?

Really, it seems to me sleep deprivation and caffeine are are taking their toll!!! 😞

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 22, 2016

Member

Yeah I think we've all learned a lot this GSOC, many things to improve for the next one :-)

I'd love to see the single insert/remove/update/activate/deactivate of some sort. If my little goofy shader experiment should be possible to add or remove based on the player eating a mushroom I figure that should be done without that full 24 item redo

The good news is that this is entirely new functionality, so it doesn't really mess with anything existing. We could wait and merge it later or merge now then aim to improve before next release, or even mark it as an "incubating" feature exempted from SemVer ("Use with caution - may change" thing) - lots of big projects do that, like Gradle :-)

Actually, I bet we could even indicate that in the @API annotation, so generated documentation would reflect it. Cool! Although yeah more complex annotations as well, alas.

For a node add function (with a specific spot in mind) would it work to pass an extra variable to indicate the name of the prior node to add the new node after? Throwing an error if said prior node is not found? Then as you mentioned on Slack document the available base nodes somewhere?

That's not perfect either, but this is a huge system we underestimated, I suspect we'll keep working on it for quite a while. On the plus side maybe we can eventually get to the promised land of only considering shaders matching a given OpenGL level somehow :D

Later on when it becomes a true graph maybe we can have a nice system that can figure it out itself akin to the world gen facets.

Member

Cervator commented Aug 22, 2016

Yeah I think we've all learned a lot this GSOC, many things to improve for the next one :-)

I'd love to see the single insert/remove/update/activate/deactivate of some sort. If my little goofy shader experiment should be possible to add or remove based on the player eating a mushroom I figure that should be done without that full 24 item redo

The good news is that this is entirely new functionality, so it doesn't really mess with anything existing. We could wait and merge it later or merge now then aim to improve before next release, or even mark it as an "incubating" feature exempted from SemVer ("Use with caution - may change" thing) - lots of big projects do that, like Gradle :-)

Actually, I bet we could even indicate that in the @API annotation, so generated documentation would reflect it. Cool! Although yeah more complex annotations as well, alas.

For a node add function (with a specific spot in mind) would it work to pass an extra variable to indicate the name of the prior node to add the new node after? Throwing an error if said prior node is not found? Then as you mentioned on Slack document the available base nodes somewhere?

That's not perfect either, but this is a huge system we underestimated, I suspect we'll keep working on it for quite a while. On the plus side maybe we can eventually get to the promised land of only considering shaders matching a given OpenGL level somehow :D

Later on when it becomes a true graph maybe we can have a nice system that can figure it out itself akin to the world gen facets.

}
@Override
public void setShadowMapNode(ShadowMapNode shadowNode) {

This comment has been minimized.

@msteiger

msteiger Aug 23, 2016

Member

I'm sceptical that this method would be in WorldRenderer. What's the rationale behind it?

@msteiger

msteiger Aug 23, 2016

Member

I'm sceptical that this method would be in WorldRenderer. What's the rationale behind it?

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Aug 23, 2016

Member

Maybe I need to revert my opinion. We probably should not expose internal classes and methods, which can easily break everything to modules, since this breaks the whole idea of the sandboxing approach.

I'd leave it locally for @tdgunes to create a proof of concept, but I'd rather not use it in the final version.

Member

msteiger commented Aug 23, 2016

Maybe I need to revert my opinion. We probably should not expose internal classes and methods, which can easily break everything to modules, since this breaks the whole idea of the sandboxing approach.

I'd leave it locally for @tdgunes to create a proof of concept, but I'd rather not use it in the final version.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Aug 23, 2016

Member

Yup this is just a proof of concept to show that it works. I think we should enhance GraphicEffectsSystem to have more methods that are narrow in scope, with that single class exposed via @API (and maybe a few utility/container type classes?). I bet some of the exposed classes/packages in this PR could have something more selective allowed through a proxy method (is that a thing?) in GES instead, removing the need to whitelist their use directly.

I'm fine merging this now or later, I don't think its merging actually factors into GSOC - this PR is listed on the project submission site either way so that's probably good enough - it does work! If we merge now we should clearly mark it as temporary code / API exposure that should not be relied on and either remove it before next game release or come up with a better "incubating API" flagging approach before then and apply it :)

Ultimately we may need a few different approaches to affecting rendering at differing levels of scope:

  1. Some simple Nodes that just add a basic shader or something could probably use a basic annotation like the @RegisterPlugin for world gen plugins
  2. Some fancier things that need to be in a very specific order could use a world-gen Facet inspired system with dependency ordering (may be too much, just seems like something to look at)
  3. Really crazy stuff could offer up some larger chunks of replacement code somehow - kinda like how the sample system replaces the entire graph (if I'm understanding it correctly)

I think this PR should focus primarily on just allowing case 1. We can shrink it down to cover that better then start putting in post-GSOC issues to cover other cases, along with whatever else we want to do next. How does that sound? :-)

Edit: Maybe if we don't merge this right away we should use a new branch + PR for an alternative more narrow approach. That way this one can stick around for reference and for GSOC.

Member

Cervator commented Aug 23, 2016

Yup this is just a proof of concept to show that it works. I think we should enhance GraphicEffectsSystem to have more methods that are narrow in scope, with that single class exposed via @API (and maybe a few utility/container type classes?). I bet some of the exposed classes/packages in this PR could have something more selective allowed through a proxy method (is that a thing?) in GES instead, removing the need to whitelist their use directly.

I'm fine merging this now or later, I don't think its merging actually factors into GSOC - this PR is listed on the project submission site either way so that's probably good enough - it does work! If we merge now we should clearly mark it as temporary code / API exposure that should not be relied on and either remove it before next game release or come up with a better "incubating API" flagging approach before then and apply it :)

Ultimately we may need a few different approaches to affecting rendering at differing levels of scope:

  1. Some simple Nodes that just add a basic shader or something could probably use a basic annotation like the @RegisterPlugin for world gen plugins
  2. Some fancier things that need to be in a very specific order could use a world-gen Facet inspired system with dependency ordering (may be too much, just seems like something to look at)
  3. Really crazy stuff could offer up some larger chunks of replacement code somehow - kinda like how the sample system replaces the entire graph (if I'm understanding it correctly)

I think this PR should focus primarily on just allowing case 1. We can shrink it down to cover that better then start putting in post-GSOC issues to cover other cases, along with whatever else we want to do next. How does that sound? :-)

Edit: Maybe if we don't merge this right away we should use a new branch + PR for an alternative more narrow approach. That way this one can stick around for reference and for GSOC.

@Cervator Cervator referenced this pull request Aug 23, 2016

Merged

Add a bunch of modules to the lineup + bump libraries #2451

4 of 4 tasks complete
@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 23, 2016

Contributor

We will work it all out @Cervator. This PR can become mergeable if it goes through a regular review process rather than a rushed one. Let's either close it or not merge it, yet.

Contributor

emanuele3d commented Aug 23, 2016

We will work it all out @Cervator. This PR can become mergeable if it goes through a regular review process rather than a rushed one. Let's either close it or not merge it, yet.

@flo

This comment has been minimized.

Show comment
Hide comment
@flo

flo Aug 24, 2016

Contributor

One way to implement the node registration in an extensible way would be to introduce a RegisterRenderEngineNodesEvent.

For each node there would be then a event handler that adds its node to a list of nodes the event has. Via the priority of the event handlers we can not just determine the order in which the events get registered, but we can also provide space for modules to hook in their own events. For this purpose we would not use the constants defined in EventPriority, but would introduce our own collection of event priorities:

class RenderEngineNodePriority {
    public static final int BEFORE_SHADOW_MAP = 100000;
    public static final int SHADOW_MAP = BEFORE_SHADOW_MAP -1;
    public static final int AFTER_SHADOW_MAP = SHADOW_MAP -1;
    public static final int BEFORE_WORLD_REFLECTION = AFTER_SHADOW_MAP -100;
    public static final int WORLD_REFLECTION = BEFORE_WORLD_REFLECTION -1;
    public static final int AFTER_WORLD_REFLECTION = WORLD_REFLECTION -1;
    public static final int BEFORE_BACK_DROP_NODE = AFTER_WORLD_REFLECTION -100;
    // ... 
}

If a module wants to insert a node after the world reflection node, it would do it like this:

        @ReceiveEvent(priority = RenderEngineNodePriority.AFTER_WORLD_REFLECTION)
        public void onRegisterMyNode(RegisterRenderEngineNodesEvent event, EntityRef entity) {
            events.addNode(new MyNode());
        }

The engine would register it's node the same way.

Contributor

flo commented Aug 24, 2016

One way to implement the node registration in an extensible way would be to introduce a RegisterRenderEngineNodesEvent.

For each node there would be then a event handler that adds its node to a list of nodes the event has. Via the priority of the event handlers we can not just determine the order in which the events get registered, but we can also provide space for modules to hook in their own events. For this purpose we would not use the constants defined in EventPriority, but would introduce our own collection of event priorities:

class RenderEngineNodePriority {
    public static final int BEFORE_SHADOW_MAP = 100000;
    public static final int SHADOW_MAP = BEFORE_SHADOW_MAP -1;
    public static final int AFTER_SHADOW_MAP = SHADOW_MAP -1;
    public static final int BEFORE_WORLD_REFLECTION = AFTER_SHADOW_MAP -100;
    public static final int WORLD_REFLECTION = BEFORE_WORLD_REFLECTION -1;
    public static final int AFTER_WORLD_REFLECTION = WORLD_REFLECTION -1;
    public static final int BEFORE_BACK_DROP_NODE = AFTER_WORLD_REFLECTION -100;
    // ... 
}

If a module wants to insert a node after the world reflection node, it would do it like this:

        @ReceiveEvent(priority = RenderEngineNodePriority.AFTER_WORLD_REFLECTION)
        public void onRegisterMyNode(RegisterRenderEngineNodesEvent event, EntityRef entity) {
            events.addNode(new MyNode());
        }

The engine would register it's node the same way.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Aug 24, 2016

Contributor

Hi @flo! Thank you for your contribution.

This PR is a bit misleading because it is based on a (temporarily) list-oriented rendering pipeline. That pipeline and all its nodes will eventually become a more explicit graph with nodes connected by edges. Different types of edges will likely have different meaning. For example one type of edge might simply describe which node should be processed first. Another might describe which camera a node uses.

I'd suggest we wait until we have that functionality implemented before we get into the nitty gritty details of how it could all work. There still is a fair amount of work to do.

Contributor

emanuele3d commented Aug 24, 2016

Hi @flo! Thank you for your contribution.

This PR is a bit misleading because it is based on a (temporarily) list-oriented rendering pipeline. That pipeline and all its nodes will eventually become a more explicit graph with nodes connected by edges. Different types of edges will likely have different meaning. For example one type of edge might simply describe which node should be processed first. Another might describe which camera a node uses.

I'd suggest we wait until we have that functionality implemented before we get into the nitty gritty details of how it could all work. There still is a fair amount of work to do.

@flo

This comment has been minimized.

Show comment
Hide comment
@flo

flo Aug 25, 2016

Contributor

@emanuele3d ah I see. Thanks for the info.

Contributor

flo commented Aug 25, 2016

@emanuele3d ah I see. Thanks for the info.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Nov 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Nov 19, 2016

Hooray Jenkins reported success with all tests good!

@flo

This comment has been minimized.

Show comment
Hide comment
@flo

flo Jan 21, 2017

Contributor

I suggest we close this PR, any objections?

If someone has something he can always create a new one.

Contributor

flo commented Jan 21, 2017

I suggest we close this PR, any objections?

If someone has something he can always create a new one.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jan 22, 2017

Contributor

I agree.

Contributor

emanuele3d commented Jan 22, 2017

I agree.

@flo flo closed this Jan 22, 2017

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