Skip to content
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

Prevent showing wall connections in unexplored terrain #17233

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@abmyii
Copy link
Contributor

abmyii commented Oct 14, 2019

Fixes #17228. I'm not sure if I put the condition in the correct condition block since now rather than simply showing only the buildBlocked sprite (red cross-hatched sprite), the wall sprite (e.g. sandbag) is shown above the buildBlocked sprite. Also, my condition is that the cell has to be explored, not necessarily visible - I'm not sure if it should be changed to visible cells only.

I noticed too late that I worked off the master branch rather than the bleed branch, so apologies for that.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 14, 2019

I noticed too late that I worked off the master branch rather than the bleed branch, so apologies for that.

This is a problem - github doesn't allow the merge branch to be changed, and we can't merge commits into master. Can you please re-file this against bleed?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 14, 2019

How would I go about doing that? Sorry for the (probably) basic question but I am still unfamiliar with the PR/commit system.

@abmyii abmyii force-pushed the abmyii:wallplacing_changes branch from e6e3683 to 4c29b60 Oct 14, 2019
@abmyii abmyii changed the base branch from master to bleed Oct 14, 2019
@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 14, 2019

Like so? It is now failing the check for some reason.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 14, 2019

Weird. I can only guess since you force pushed before changing the base travis and appveyor thought the PR is not mergable. Can you try force pushing again? (To trigger a build.)

@abmyii abmyii force-pushed the abmyii:wallplacing_changes branch from 4c29b60 to a961af9 Oct 14, 2019
@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 14, 2019

Ah right, that worked. I changed the commit message and pushed.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 17, 2019

Does the PR require anything else to be done?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 20, 2019

Any updates?

Copy link
Contributor

abc013 left a comment

LGTM 👍

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 20, 2019

Great, I hope it gets merged some time today!

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 20, 2019

We are extremely short on maintainer manpower currently, and the effort that we do have has been trying to focus on the stable release. I'm sorry to say this means that PR reviews will generally be very slow.

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 20, 2019

No problem, I'll try to exercise more patience. If I can do anything to help please let me know!

Copy link
Member

abcdefg30 left a comment

Lgtm otherwise.

@@ -251,7 +252,7 @@ IEnumerable<IRenderable> IOrderGenerator.RenderAboveShroud(WorldRenderer wr, Wor

footprint.Add(topLeft, MakeCellType(AcceptsPlug(topLeft, plugInfo)));
}
else if (lineBuildInfo != null)
else if (owner.Shroud.IsExplored(topLeft) && lineBuildInfo != null)

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Oct 21, 2019

Member

Can we please swap the two conditions? lineBuildInfo != null && owner.Shroud.IsExplored(topLeft)
IsExplored is relatively expensive compared to a simple null check, so it makes sense to check that first and not do the call to IsExplored if it is not needed.

This comment has been minimized.

Copy link
@abmyii

abmyii Oct 21, 2019

Author Contributor

Sure, I'll do that.

@abmyii abmyii force-pushed the abmyii:wallplacing_changes branch from a961af9 to bfac051 Oct 21, 2019
@abcdefg30 abcdefg30 force-pushed the abmyii:wallplacing_changes branch from bfac051 to 6f8e11f Oct 21, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 21, 2019

Testing revealed another issue: The linebuild is still shown when the target cell is obscured.
grafik
(Testcase allies06a.)

I figured describing exactly how to fix this takes longer than quickly adding the changes I wanted myself, I hope you don't mind.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 21, 2019

@abc013 (or someone else): Can you please confirm your 👍 with the new changes?

@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 21, 2019

Ah I see, didn't catch that. Thanks a lot!

@abmyii abmyii force-pushed the abmyii:wallplacing_changes branch from 6f8e11f to e483c68 Oct 30, 2019
@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Oct 31, 2019

Is there anything else this PR is missing?

@teinarss teinarss merged commit 023750d into OpenRA:bleed Nov 1, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abmyii

This comment has been minimized.

Copy link
Contributor Author

abmyii commented Nov 1, 2019

Awesome, thanks all for the help!

@reaperrr reaperrr mentioned this pull request Nov 8, 2019
12 of 19 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.