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

Backdrop and Skybands Nodes for DAG #2362

Merged
merged 7 commits into from Jun 22, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Jun 18, 2016

Contains

Two new nodes for DAG (Directed Acyclic Graph), wrapping tasks that are related with rendering Sky.

How to test

Check if there are any weirdness related to rendering of Sky.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 18, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 18, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes added the GSoC label Jun 18, 2016

@tdgunes tdgunes self-assigned this Jun 18, 2016

@tdgunes tdgunes added the Rendering label Jun 18, 2016

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jun 19, 2016

Member

I can already feel #97 moving ever so slightly! Aurora Borealis here we come! 🎆

Tested it out just a bit to look at the sky in varying conditions (changing time of day, video settings) and it still looks like good ole sky.

Member

Cervator commented Jun 19, 2016

I can already feel #97 moving ever so slightly! Aurora Borealis here we come! 🎆

Tested it out just a bit to look at the sky in varying conditions (changing time of day, video settings) and it still looks like good ole sky.

import static org.lwjgl.opengl.GL11.glDisable;
import static org.lwjgl.opengl.GL11.glEnable;
import static org.lwjgl.opengl.GL11.glStencilFunc;
import static org.lwjgl.opengl.GL11.*;

This comment has been minimized.

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

I think the project's coding standards are against this. Did you use Checkstyle?

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

I think the project's coding standards are against this. Did you use Checkstyle?

This comment has been minimized.

@tdgunes

tdgunes Jun 19, 2016

Member

Just rechecked. Seems like import static statements with wildcards are not against CheckStyle.

@tdgunes

tdgunes Jun 19, 2016

Member

Just rechecked. Seems like import static statements with wildcards are not against CheckStyle.

This comment has been minimized.

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

Let's ask @msteiger or @Cervator, they know better.

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

Let's ask @msteiger or @Cervator, they know better.

This comment has been minimized.

@Cervator

Cervator Jun 19, 2016

Member

Static imports are considered "different" somehow, but I can't actually remember why. A quick Google search and a page find for "static" gets you a little bit of reasoning: http://stackoverflow.com/questions/2067158/what-is-the-proper-style-for-listing-imports-in-java

That thread also finally made me realize why regular import star is bad - not just for some optimization reason, but if new classes are added that would suddenly qualify due to the wildcard bad things can suddenly happen. Which differs with static imports... somehow.

@Cervator

Cervator Jun 19, 2016

Member

Static imports are considered "different" somehow, but I can't actually remember why. A quick Google search and a page find for "static" gets you a little bit of reasoning: http://stackoverflow.com/questions/2067158/what-is-the-proper-style-for-listing-imports-in-java

That thread also finally made me realize why regular import star is bad - not just for some optimization reason, but if new classes are added that would suddenly qualify due to the wildcard bad things can suddenly happen. Which differs with static imports... somehow.

This comment has been minimized.

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

@Cervator: so..... import star for static imports is ok? Can you confirm?

@emanuele3d

emanuele3d Jun 19, 2016

Contributor

@Cervator: so..... import star for static imports is ok? Can you confirm?

This comment has been minimized.

@Cervator

Cervator Jun 19, 2016

Member

Can't confirm completely, no. I wouldn't go out of my way to star import statics if they're split out already, but it may make sense in general. @immortius could probably call it, or @flo / @msteiger

@Cervator

Cervator Jun 19, 2016

Member

Can't confirm completely, no. I wouldn't go out of my way to star import statics if they're split out already, but it may make sense in general. @immortius could probably call it, or @flo / @msteiger

This comment has been minimized.

@msteiger

msteiger Jun 22, 2016

Member

I don't see any advantage of using wildcard imports. Any reasonable IDE would fold imports, so you don't even see them. Most notable drawback imho is that you don't immediately see which class is used if multiple wildcards are used. You can also run into conflicts if two classes with the same name are imported.

@msteiger

msteiger Jun 22, 2016

Member

I don't see any advantage of using wildcard imports. Any reasonable IDE would fold imports, so you don't even see them. Most notable drawback imho is that you don't immediately see which class is used if multiple wildcards are used. You can also run into conflicts if two classes with the same name are imported.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 19, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jun 19, 2016

Contributor

@msteiger! We will be waiting for your words of wisdom.
@tdgunes: do you already have a branch for the next few nodes? To familiarize myself with what's coming?

Contributor

emanuele3d commented Jun 19, 2016

@msteiger! We will be waiting for your words of wisdom.
@tdgunes: do you already have a branch for the next few nodes? To familiarize myself with what's coming?

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes Jun 19, 2016

Member

@emanuele3d Yes! I have prepost-composition nodes in this branch.

Member

tdgunes commented Jun 19, 2016

@emanuele3d Yes! I have prepost-composition nodes in this branch.

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Jun 22, 2016

Member

Looks good to me in general. Anything specific (apart from the wildcard imports) that I can contribute to?

Member

msteiger commented Jun 22, 2016

Looks good to me in general. Anything specific (apart from the wildcard imports) that I can contribute to?

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jun 22, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jun 22, 2016

Hooray Jenkins reported success with all tests good!

@msteiger msteiger merged commit fa003d3 into MovingBlocks:develop Jun 22, 2016

1 check passed

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

@GooeyHub GooeyHub removed the in progress label Jun 22, 2016

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger Jun 22, 2016

Member

Thanks!

Member

msteiger commented Jun 22, 2016

Thanks!

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

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jun 22, 2016

Contributor

Yay!!!

Contributor

emanuele3d commented Jun 22, 2016

Yay!!!

@tdgunes tdgunes changed the title from [RFR] Backdrop and Skybands Nodes for DAG to Backdrop and Skybands Nodes for DAG Jun 23, 2016

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