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

Wrapping ShadowMap and WorldReflection into a "Node" #2325

Merged
merged 17 commits into from Jun 18, 2016

Conversation

Projects
None yet
6 participants
@tdgunes
Member

tdgunes commented May 25, 2016

No description provided.

@tdgunes tdgunes closed this May 25, 2016

@GooeyHub GooeyHub removed the in progress label May 25, 2016

@tdgunes tdgunes deleted the tdgunes:dag branch May 25, 2016

@tdgunes tdgunes restored the tdgunes:dag branch May 25, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 25, 2016

Member

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

Member

GooeyHub commented May 25, 2016

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

@tdgunes tdgunes reopened this May 25, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 25, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 25, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 30, 2016

Member

Went ahead and tested this, since it was here, although I had no idea what to really test :D

Ran around a world a bit looking at the pretty flowers, then cycled through the various debug rendering modes, including some pretty trippy ones (more than I remember! Neato, almost have LSD-mode in there again)

Is this related to some discussion elsewhere, like on Slack/IRC/Forum/GSOC conference meetings? I figure it might be from the empty description. What are our plans with this PR - just for reference or merge it soon? :-)

Member

Cervator commented May 30, 2016

Went ahead and tested this, since it was here, although I had no idea what to really test :D

Ran around a world a bit looking at the pretty flowers, then cycled through the various debug rendering modes, including some pretty trippy ones (more than I remember! Neato, almost have LSD-mode in there again)

Is this related to some discussion elsewhere, like on Slack/IRC/Forum/GSOC conference meetings? I figure it might be from the empty description. What are our plans with this PR - just for reference or merge it soon? :-)

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 30, 2016

Contributor

The goal of this PR is to implement a barebone DAG class(es) and have -one- rendering step wrapped into a node. Currently the targeted step is the one generating the main light's shadow map.

That been said, @tdgunes had his thesis discussion today I think, the last hurdle before graduation. So, he might have needed a bit of time to prepare that in the past week or so and he might need a bit of time to get over the celebrations in the next few days. =D

Contributor

emanuele3d commented May 30, 2016

The goal of this PR is to implement a barebone DAG class(es) and have -one- rendering step wrapped into a node. Currently the targeted step is the one generating the main light's shadow map.

That been said, @tdgunes had his thesis discussion today I think, the last hurdle before graduation. So, he might have needed a bit of time to prepare that in the past week or so and he might need a bit of time to get over the celebrations in the next few days. =D

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 30, 2016

Contributor

Sorry, I didn't address one of your points @Cervator: indeed the goal of this PR has been set in one of our live meeting, first on appear.in and then via skype due to technical problems. @tdgunes also already has some feedback regarding this PR, discussed at our last meeting, and we are awaiting a new commit to incorporate that feedback.

In general Martin, Taha and I have agreed that it will be preferable to have small(ish) weekly or bi-weekly incremental changes to the rendering engine instead of developing on a separate branch for a long time to then spend a long time integrating everything. So, we don't expect any individual PR to stay open for too long. Perhaps this first one might take a bit longer but mostly because Taha is busy wrapping up four years of study. =)

Contributor

emanuele3d commented May 30, 2016

Sorry, I didn't address one of your points @Cervator: indeed the goal of this PR has been set in one of our live meeting, first on appear.in and then via skype due to technical problems. @tdgunes also already has some feedback regarding this PR, discussed at our last meeting, and we are awaiting a new commit to incorporate that feedback.

In general Martin, Taha and I have agreed that it will be preferable to have small(ish) weekly or bi-weekly incremental changes to the rendering engine instead of developing on a separate branch for a long time to then spend a long time integrating everything. So, we don't expect any individual PR to stay open for too long. Perhaps this first one might take a bit longer but mostly because Taha is busy wrapping up four years of study. =)

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 30, 2016

Member

I'd double-thumbs-up that follow-up post, but there isn't a reaction-emoticon for that so I'll post a full comment just to say thanks for all the details! :-)

Member

Cervator commented May 30, 2016

I'd double-thumbs-up that follow-up post, but there isn't a reaction-emoticon for that so I'll post a full comment just to say thanks for all the details! :-)

@rzats rzats added the GSoC label Jun 3, 2016

@tdgunes tdgunes changed the title from Initial DirectedAcylicGraph structures to Initial DirectedAcylicGraph structures and ShadowMapping as a Node Jun 9, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 9, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 9, 2016

Hooray Jenkins reported success with all tests good!

@@ -195,13 +204,25 @@ private void initRenderingSupport() {
shaderManager.initShaders();
postProcessor.initializeMaterials();
initMaterials();
context.put(WorldRenderer.class, this);

This comment has been minimized.

@emanuele3d

emanuele3d Jun 14, 2016

Contributor

Can this be placed at the very end of the constructor?

@emanuele3d

emanuele3d Jun 14, 2016

Contributor

Can this be placed at the very end of the constructor?

This comment has been minimized.

@tdgunes

tdgunes Jun 14, 2016

Member

At this stage, every node depends on WorldRendererImpl, wouldn't be better to not make constructors overly crowded?

@tdgunes

tdgunes Jun 14, 2016

Member

At this stage, every node depends on WorldRendererImpl, wouldn't be better to not make constructors overly crowded?

This comment has been minimized.

@emanuele3d

emanuele3d Jun 15, 2016

Contributor

Ok, let's leave it there then.

@emanuele3d

emanuele3d Jun 15, 2016

Contributor

Ok, let's leave it there then.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jun 14, 2016

Contributor

Code reviewed and feedback provided up to commit 98687b4.

Contributor

emanuele3d commented Jun 14, 2016

Code reviewed and feedback provided up to commit 98687b4.

@tdgunes tdgunes changed the title from [Ready for Review] Wrapping ShadowMap and WorldReflection into a "Node" to [Work in Progress] Wrapping ShadowMap and WorldReflection into a "Node" Jun 14, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 15, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 15, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from [Work in Progress] Wrapping ShadowMap and WorldReflection into a "Node" to [Ready for Review] Wrapping ShadowMap and WorldReflection into a "Node" Jun 15, 2016

@@ -39,6 +41,15 @@
float BLOCK_LIGHT_SUN_POW = 0.96f;
float BLOCK_INTENSITY_FACTOR = 0.7f;
// TODO: appropriate javadocs

This comment has been minimized.

@emanuele3d

emanuele3d Jun 15, 2016

Contributor

Indeed! =)

@emanuele3d

emanuele3d Jun 15, 2016

Contributor

Indeed! =)

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 16, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 16, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title from [Ready for Review] Wrapping ShadowMap and WorldReflection into a "Node" to [Ready for Merge?] Wrapping ShadowMap and WorldReflection into a "Node" Jun 16, 2016

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Jun 16, 2016

Member

Thanks for updating the code base @tdgunes. I sent a PR to your dag branch about the NodeCreator.

I'm hesitant to add new methods to WorldRenderer, unless really needed:

  • The time-dependent part (getSecondsSinceLastFrame) could/should be part of the update cycle rather than being inside the rendering.
  • getMaterial could be replaced with injection of AssetManager. This also provides other convenience methods as well as other Asset types.
  • isFirstRenderingStageForCurrentFrame is too specific. Dependencies and ordering to other nodes can be dealt later with.
  • renderChunks() looks like an internal(=private) method to me. What you're interested in is another render pass with different parameters, right?
Member

msteiger commented Jun 16, 2016

Thanks for updating the code base @tdgunes. I sent a PR to your dag branch about the NodeCreator.

I'm hesitant to add new methods to WorldRenderer, unless really needed:

  • The time-dependent part (getSecondsSinceLastFrame) could/should be part of the update cycle rather than being inside the rendering.
  • getMaterial could be replaced with injection of AssetManager. This also provides other convenience methods as well as other Asset types.
  • isFirstRenderingStageForCurrentFrame is too specific. Dependencies and ordering to other nodes can be dealt later with.
  • renderChunks() looks like an internal(=private) method to me. What you're interested in is another render pass with different parameters, right?
Merge pull request #2 from msteiger/dag
Internalize node instance creation

@tdgunes tdgunes changed the title from [Ready for Merge?] Wrapping ShadowMap and WorldReflection into a "Node" to Wrapping ShadowMap and WorldReflection into a "Node" Jun 16, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 16, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 16, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jun 17, 2016

Member

@msteiger: Thanks for your feedback! For getMaterial(), It would be great if you can give an example. It seems like static Assets helper class is used much extensively in the codebase rather then AssetManager.getAsset().

Member

tdgunes commented Jun 17, 2016

@msteiger: Thanks for your feedback! For getMaterial(), It would be great if you can give an example. It seems like static Assets helper class is used much extensively in the codebase rather then AssetManager.getAsset().

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 18, 2016

Member

Yes, I would already prefer using Assets over WorldRenderer, but getting the actual asset manager from the context and using that would be even better (more flexibility, better testing caps.)

Member

GooeyHub commented Jun 18, 2016

Yes, I would already prefer using Assets over WorldRenderer, but getting the actual asset manager from the context and using that would be even better (more flexibility, better testing caps.)

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jun 18, 2016

Member

I think that last @GooeyHub was secretly @msteiger in disguise, or simply having forgotten to change accounts back again :D

I'll aim to test and merge this shortly to produce a new Omega build we can use for some play testing in a couple hours, unless there are any objections :-)

Member

Cervator commented Jun 18, 2016

I think that last @GooeyHub was secretly @msteiger in disguise, or simply having forgotten to change accounts back again :D

I'll aim to test and merge this shortly to produce a new Omega build we can use for some play testing in a couple hours, unless there are any objections :-)

@Cervator Cervator merged commit a1e0099 into MovingBlocks:develop Jun 18, 2016

1 check passed

default Build finished. 503 tests run, 0 skipped, 0 failed.
Details

Cervator added a commit that referenced this pull request Jun 18, 2016

@Cervator Cervator removed the in progress label Jun 18, 2016

@Cervator Cervator added this to the Alpha 2 milestone Jun 18, 2016

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Jun 19, 2016

Member

Whoops! That's correct - it was me.

Member

msteiger commented Jun 19, 2016

Whoops! That's correct - it was me.

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