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

Finalize SetWireframe StateChange #2421

Merged
merged 7 commits into from Jul 31, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented Jul 27, 2016

Contains

As title mentions, finalizes a Wireframe state change for All obvious State Changes implemented

How to test

By checking the log and observing wireframe debug mode.

FYI: Debug mode is toggled by F3 key and wireframe with F9.

An example screenshot when wireframe is enabled:

screen shot 2016-07-27 at 04 31 00

Eliminations must be seen in the log:

demonstration

Important note:

This PR also includes setViewportSizeOf PR. Please just focus on last commit.

@tdgunes tdgunes changed the title from Finalize SetWireframe StateChange to [RFR] Finalize SetWireframe StateChange Jul 27, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 27, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 27, 2016

Hooray Jenkins reported success with all tests good!

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jul 27, 2016

Member

Neat screenie! Apparently 3D wizards keep spiders as familiars :D

Member

Cervator commented Jul 27, 2016

Neat screenie! Apparently 3D wizards keep spiders as familiars :D

@tdgunes tdgunes changed the title from [RFR] Finalize SetWireframe StateChange to [WIP] Finalize SetWireframe StateChange Jul 28, 2016

@tdgunes tdgunes changed the title from [WIP] Finalize SetWireframe StateChange to [RFR] Finalize SetWireframe StateChange Jul 29, 2016

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 29, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 29, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 29, 2016

Contributor

Too late for me to review this, I better go to bed. I'll review it over the weekend.

Contributor

emanuele3d commented Jul 29, 2016

Too late for me to review this, I better go to bed. I'll review it over the weekend.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 30, 2016

Contributor

Good work on this one. Very few minor issues to address. Let's line it up for landing!

Contributor

emanuele3d commented Jul 30, 2016

Good work on this one. Very few minor issues to address. Let's line it up for landing!

@tdgunes tdgunes referenced this pull request Jul 30, 2016

Merged

Adds SetViewportSizeOf StateChange #2419

1 of 1 task complete
@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub Jul 30, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented Jul 30, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 30, 2016

Contributor

Changes introduced in 53ca1a4 look good and address the remaining issues. Please @tdgunes solve the conflicts and please @Cervator be informed that this PR is on final approach for landing. =)

Contributor

emanuele3d commented Jul 30, 2016

Changes introduced in 53ca1a4 look good and address the remaining issues. Please @tdgunes solve the conflicts and please @Cervator be informed that this PR is on final approach for landing. =)

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jul 30, 2016

Member

The conflicts may be total non-issues due to the import tweaking I did myself. If that's all I'm happy to resolve that and merge - will take a look a little later tonight :-)

Thanks for the updates!

Member

Cervator commented Jul 30, 2016

The conflicts may be total non-issues due to the import tweaking I did myself. If that's all I'm happy to resolve that and merge - will take a look a little later tonight :-)

Thanks for the updates!

@Cervator Cervator merged commit 53ca1a4 into MovingBlocks:develop Jul 31, 2016

1 check passed

default Build finished.
Details

Cervator added a commit that referenced this pull request Jul 31, 2016

Merge PR #2421 by @tdgunes - rendering work.
Conflicts:
	engine/src/main/java/org/terasology/rendering/world/WorldRendererImpl.java - simple imports conflict

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

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jul 31, 2016

Member

I have gazed upon the arcane spiderweb of 3d wizardry, and it was good. I now know kung fu.

Conflicts were benign as expected: import clash. Found a new/modified file in this PR that then also needed a tweak on top since it was still using the dependency I changed in #2386 (done in 7af48ae)

As noted in the top post this also included #2419 which I guess may or may not have outstanding comments? In any case I would grab latest including the conflict resolution and apply any needed tweaks on top :-)

Member

Cervator commented Jul 31, 2016

I have gazed upon the arcane spiderweb of 3d wizardry, and it was good. I now know kung fu.

Conflicts were benign as expected: import clash. Found a new/modified file in this PR that then also needed a tweak on top since it was still using the dependency I changed in #2386 (done in 7af48ae)

As noted in the top post this also included #2419 which I guess may or may not have outstanding comments? In any case I would grab latest including the conflict resolution and apply any needed tweaks on top :-)

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

@tdgunes tdgunes changed the title from [RFR] Finalize SetWireframe StateChange to Finalize SetWireframe StateChange Jul 31, 2016

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d Jul 31, 2016

Contributor

Hmmm... this merge was a bit of a mess - my fault.

What I could see specifically from this PR was ready for merge, but I hadn't realized that this PR was based on another one that wasn't quite ready to go yet.

@Cervator: how can we avoid this happening again?

Contributor

emanuele3d commented Jul 31, 2016

Hmmm... this merge was a bit of a mess - my fault.

What I could see specifically from this PR was ready for merge, but I hadn't realized that this PR was based on another one that wasn't quite ready to go yet.

@Cervator: how can we avoid this happening again?

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator Jul 31, 2016

Member

@emanuele3d that was what I was getting at on Slack :-) While functionally distinct PRs may ease review if they overlap in code this sort of thing can happen. Not a lot to do about it other than keeping stuff more insulated or closing older PRs as they're superseded by newer ones (or I guess keep them open but with the understanding that they're subordinate to some further along PR)

Git and GitHub doesn't really care what we do about PRs so long as all the right commits end up in the right branch eventually.

Member

Cervator commented Jul 31, 2016

@emanuele3d that was what I was getting at on Slack :-) While functionally distinct PRs may ease review if they overlap in code this sort of thing can happen. Not a lot to do about it other than keeping stuff more insulated or closing older PRs as they're superseded by newer ones (or I guess keep them open but with the understanding that they're subordinate to some further along PR)

Git and GitHub doesn't really care what we do about PRs so long as all the right commits end up in the right branch eventually.

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