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

Overhaul actor render and sprite bounds. #14482

Merged
merged 16 commits into from Dec 11, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Dec 9, 2017

The successive layers of workarounds and hacks in the render and selection bounds code recently passed their breaking point. This PR aims to fix the underlying problem once and for all by throwing everything out and starting from a new, modern, base.

Most of the commits should be self explanatory and simple enough to review if you go through them one by one. Things turn awful once we hit the the need to separate the selection bounds, and I couldn't find a cleaner way to separate out the commits without leaving broken code or regressions. The pain of reviewing should be worth it (and minor compared to the pain of writing it).

Testing should use the new /showscreenmap command: the green rectangles are the areas used to work out when actors should be rendered on the screen, and the red rectangles mark the areas that are used for mouse interaction. The visual selection boxes may be different again, as in the case of infantry. When I was testing I found it useful to move a unit's turret a stupid distance (several cells) away from the body to verify correct behaviour on turning and screen edge.

I expect there may be some minor regressions in selection box sizes (the algorithm for automatic sizing has changed), but these are best found by a fresh set of eyes, and can be easily fixed by defining overrides in yaml. Third party modders will see loud errors/lint warnings if they have actors with too few or too many IMouseBounds traits.

Closes #4617
Fixes #5552
Fixes #6987
Fixes #11115
Fixes #12119
Fixes #14449
Fixes #14455
Fixes #14462
Fixes #14481
Fixes #14483
Fixes #14493
Supersedes #13926
Supersedes #14450
Implements most of #4783

pchote added some commits Dec 3, 2017

Add ISpriteSequence.Bounds and Animation(WithOffset).ScreenBounds.
The bounds define the smallest Rectangle that covers all frames within a sequence.
Add IModel.AggregateBounds and ModelAnimation.ScreenBounds.
The bounds define the smallest Rectangle that covers all
rotations of all frames within a model sequence.
Add ScreenBounds method to IRender interface.
This method is expected to return Rectangles that cover
the region where Renderables returned by Render may exist.

@pchote pchote added this to the Next release milestone Dec 9, 2017

@pchote pchote requested review from reaperrr, RoosterDragon and penev92 Dec 9, 2017

@@ -75,6 +75,12 @@ IEnumerable<IRenderable> IRender.Render(Actor self, WorldRenderer wr)
return !Info.RequiresSelection ? RenderInner(self, wr) : SpriteRenderable.None;
}
IEnumerable<Rectangle> IRender.ScreenBounds(Actor self, WorldRenderer wr)

This comment has been minimized.

@reaperrr

reaperrr Dec 9, 2017

Contributor

Not sure, just a question (I already noticed this while working on my PR): Is using IRender instead of IRenderAboveShroud in this trait intentional?
Because it uses IRenderAboveShroudWhenSelected when Info.RequiresSelection is true, so this feels a bit inconsistent (note: I haven't checked yet whether a following commit adresses this).

@reaperrr

reaperrr Dec 9, 2017

Contributor

Not sure, just a question (I already noticed this while working on my PR): Is using IRender instead of IRenderAboveShroud in this trait intentional?
Because it uses IRenderAboveShroudWhenSelected when Info.RequiresSelection is true, so this feels a bit inconsistent (note: I haven't checked yet whether a following commit adresses this).

This comment has been minimized.

@pchote

pchote Dec 9, 2017

Member

This was intentional because IRenderAboveShroudWhenSelected is invoked directly instead of going via the screen map. #14082 complicates matters a bit, but i'd prefer to leave the discussion and implementation to that or another PR -- expanding the render bounds to include very large range circles doesn't feel like a good approach to me, and any other approach would make this PR even bigger.

@pchote

pchote Dec 9, 2017

Member

This was intentional because IRenderAboveShroudWhenSelected is invoked directly instead of going via the screen map. #14082 complicates matters a bit, but i'd prefer to leave the discussion and implementation to that or another PR -- expanding the render bounds to include very large range circles doesn't feel like a good approach to me, and any other approach would make this PR even bigger.

{
var selectable = actorInfo.Value.TraitInfos<SelectableInfo>().Count();
var interactable = actorInfo.Value.TraitInfos<InteractableInfo>().Count();
if (selectable > 0 && selectable != interactable)

This comment has been minimized.

@reaperrr

reaperrr Dec 9, 2017

Contributor

Won't this emit a warning even if interactable == 0?

@reaperrr

reaperrr Dec 9, 2017

Contributor

Won't this emit a warning even if interactable == 0?

This comment has been minimized.

@pchote

pchote Dec 9, 2017

Member

SelectableInfo inherits InteractableInfo so are also included in the interactable count. The idea here is to make sure that there are either only Interactable traits (selectable == 0), or that all Interactable traits are actually Selectable (selectable == interactable).

@pchote

pchote Dec 9, 2017

Member

SelectableInfo inherits InteractableInfo so are also included in the interactable count. The idea here is to make sure that there are either only Interactable traits (selectable == 0), or that all Interactable traits are actually Selectable (selectable == interactable).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 9, 2017

Member

Updated [Desc].

Member

pchote commented Dec 9, 2017

Updated [Desc].

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 9, 2017

Member

Note that this intentionally doesn't set up the map editor to use the new mouse interaction bounds. That requires a whole new set of code that makes more sense as a followup PR considering that the bounds were already wrong there to start with.

Member

pchote commented Dec 9, 2017

Note that this intentionally doesn't set up the map editor to use the new mouse interaction bounds. That requires a whole new set of code that makes more sense as a followup PR considering that the bounds were already wrong there to start with.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 9, 2017

Contributor

MiG shadow still disappears as soon as the mig itself is off-screen.

Edit: Same seems to be true for voxel aircraft in TS.

ra-airc1
banshee1

Contributor

reaperrr commented Dec 9, 2017

MiG shadow still disappears as soon as the mig itself is off-screen.

Edit: Same seems to be true for voxel aircraft in TS.

ra-airc1
banshee1

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 9, 2017

Contributor

The TD Landing Craft (not OLDLST, but the one with custom shp) and pretty much all trees and rocks in RA/TD have now too large interactable area, but IMO those can be dealt with in a follow-up.

Couldn't find any other regressions or problems. I'd like to see aircraft shadows fixed, but otherwise, this looks good to go to me. 👍

Contributor

reaperrr commented Dec 9, 2017

The TD Landing Craft (not OLDLST, but the one with custom shp) and pretty much all trees and rocks in RA/TD have now too large interactable area, but IMO those can be dealt with in a follow-up.

Couldn't find any other regressions or problems. I'd like to see aircraft shadows fixed, but otherwise, this looks good to go to me. 👍

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Dec 9, 2017

Contributor

Actually, I did come across a regression just now: The D2k harv "pick me up" symbol appeared somewhere in the middle of the shellmap.

Contributor

reaperrr commented Dec 9, 2017

Actually, I did come across a regression just now: The D2k harv "pick me up" symbol appeared somewhere in the middle of the shellmap.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 9, 2017

Member
  • Fixed aircraft shadows by adding c5dfa42
  • Fixed selection decorations by removing wr.ScreenPxPosition(self.CenterPosition) (these are now baked into the bounds).
Member

pchote commented Dec 9, 2017

  • Fixed aircraft shadows by adding c5dfa42
  • Fixed selection decorations by removing wr.ScreenPxPosition(self.CenterPosition) (these are now baked into the bounds).
@Smittytron

This comment has been minimized.

Show comment
Hide comment
@Smittytron

Smittytron Dec 10, 2017

Contributor

Civilian buildings do not appear as frozen actors on explored map at the start of a game.

Also, it seemed like units lose their targets immediately as they enter the fog of war. (Maybe this is a good thing, I'll try to enlist a live body and see if I can assess how this might impact gameplay.)

Contributor

Smittytron commented Dec 10, 2017

Civilian buildings do not appear as frozen actors on explored map at the start of a game.

Also, it seemed like units lose their targets immediately as they enter the fog of war. (Maybe this is a good thing, I'll try to enlist a live body and see if I can assess how this might impact gameplay.)

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 10, 2017

Member

Also, it seemed like units lose their targets immediately as they enter the fog of war.

This PR shouldn't be able to affect any of the targeting logic. Can you please test the same cases on bleed, and try and track down the PR that introduced it?

Member

pchote commented Dec 10, 2017

Also, it seemed like units lose their targets immediately as they enter the fog of war.

This PR shouldn't be able to affect any of the targeting logic. Can you please test the same cases on bleed, and try and track down the PR that introduced it?

pchote added some commits Dec 9, 2017

Use IRender.ScreenBounds in ScreenMap.
Traits are now required to trigger a ScreenMap update whenever they
believe that their ScreenBounds have changed.
Introduce IMouseBounds and split/rework mouse rectangles.
The render bounds for an actor now include the area covered
by bibs, shadows, and any other widgets. In many cases this
area is much larger than we really want to consider for
tooltips and mouse selection.

An optional Margin is added to Selectable to support cases
like infantry, where we want the mouse area of the actor
to be larger than the drawn selection box.
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 10, 2017

Member

Fixed the frozen actor issue. The problem was inside FrozenUnderFog.TickRender, where I was only updating the screen map inside the if (renderables == null). Fixed by caching the rectangles in the same was as the renderables and triggering a ScreenMap update for all dirty players.

Member

pchote commented Dec 10, 2017

Fixed the frozen actor issue. The problem was inside FrozenUnderFog.TickRender, where I was only updating the screen map inside the if (renderables == null). Fixed by caching the rectangles in the same was as the renderables and triggering a ScreenMap update for all dirty players.

@abcdefg30

@reaperrr is your 👍 still valid?

@reaperrr reaperrr merged commit 6633483 into OpenRA:bleed Dec 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment