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

Editor feature to edit actor ID and owner. #15551

Merged
merged 1 commit into from Nov 24, 2018

Conversation

Projects
None yet
@drogoganor
Copy link
Contributor

drogoganor commented Aug 22, 2018

actor edit

A feature to select and edit an actor's ID and owner in the editor.

I need a bit of feedback on this one, especially on the following items:

  1. I've added a public EditorActorPreview SelectedActor to EditorDefaultBrush, and HandleMouseInput() sets this if the user left-clicks an actor. I've also made the EditorViewportControllerWidget's defaultBrush public so that SelectedActor can be accessed and set by ActorEditLogic. I don't think this is a good design. I'm wondering if there's a nicer way of doing this.

  2. I've made the EditorActorLayer's Add() public in order to facilitate removing and re-adding the actor in ActorEditLogic. I'm not sure if this is a good idea.

  3. I've added an optional width constructor argument for SelectionBoxRenderable, which was hard-coded to 1. I found I needed to specify a wider width for the selection box because it was hard to see at just one pixel. But I'm not sure if this is appropriate or not. If you look closely at the image, you can see the left and right edges of the selection box are 2px extended.

  4. In order to get the type of the actor and the nice tooltip name, I've had to add another field TypeSummary to EditorActorPreview in the same vein as Tooltip. Is this OK? Also I'd like to know if the formatting of this field in the image above is OK.

  5. The color of the selection box. Right now you can see above it sort of blends in with the red buildings. Should this be another color? I tried hot pink, and that was certainly visible but...hot pink.

Cheers

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch from f85dd88 to 6441c7c Aug 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Aug 22, 2018

The formatting in the image looks good to me, although I'd maybe swap ID and Owner.
About the selection box color, have you tested neon green?

By the way, could you mabe add Health editing support as well?

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Aug 23, 2018

I've swapped ID and Owner, and also made the selection box green.

limegreen

It generally works pretty well except for tiberium as you can see, or snow:

actor edit

And here's hot pink, which blends in with the d2k desert and probably cnc desert too, but generally stands out best:

actor edit

Also there's an unrelated problem with the Bounds size for infantry units. I'm not sure how to resolve this yet:

limegreen

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Aug 23, 2018

How about blue, not cyan though I imagine cyan will blend very easily with snow.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Aug 23, 2018

the infantry bounds is definitely related to #15148 and all it's followups.

Regarding the selection box, blinking two colors on the opposite end of the spectrum (red-green/black-white/etc) might make the most sense.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 23, 2018

the infantry bounds is definitely related to #15148 and all it's followups.

Kindof yes and kindof no - the ActorPreviews have never really needed a concept for interactable bounds, so this was never implemented - in its absence the only alternative is the size of the renderable which is indeed inflated for these infantry sprites. This also affects the editor sidebar.

This all happened before the recent interaction bounds rework, so it should now be a lot easier to hook up the bounds defined in the actor rules.

@KOYK

This comment has been minimized.

Copy link
Contributor

KOYK commented Aug 23, 2018

Can you also add the ability to rotate the actor?

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Aug 24, 2018

Regarding blue selection box:

actor edit

It's OK too but blends in with grey a bit.

I'll give the alternating colors a go today.

Regarding obtaining the interaction bounds - I've tried getting InteractableInfo for the actor and trying to use the Bounds or DecorationBounds. But it seems to be almost always null. I'll reach out on IRC for some help.

As for rotating the actor, I've been told there was some prior work done to be able to mark certain traits(?) to be exposed in the editor like this. This would be better than hard-coding controls for certain traits. However, I'm not fully up to speed on how this was supposed to work. You might need to seek out comment from a senior dev.

@@ -76,6 +79,8 @@ public EditorActorPreview(WorldRenderer worldRenderer, string id, ActorReference
Tooltip = (tooltip == null ? " < " + Info.Name + " >" : tooltip.Name) + "\n" + owner.Name + " (" + owner.Faction + ")"
+ "\nID: " + ID + "\nType: " + Info.Name;

TypeSummary = Info.Name + (tooltip != null ? " (" + tooltip.Name + ")" : string.Empty);

This comment has been minimized.

@Mailaender

Mailaender Sep 10, 2018

Member

This adds a double (( for things like the camera where there is already a ( in the name.

@Mailaender
Copy link
Member

Mailaender left a comment

Works as promised.

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Sep 10, 2018

I'll give the alternating colors a go today.

How about using the player color remaps? They should blend in at all times.

@RobotCaleb

This comment has been minimized.

Copy link
Contributor

RobotCaleb commented Oct 3, 2018

Why not a 3 pixel wide selection caret?
black-white-black

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch 2 times, most recently from 6413fba to 6fc6a7a Oct 18, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Oct 18, 2018

Updated to use a white selection box and flash the selected actor with the highlight palette every 500ms.

image

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch from 6fc6a7a to 38bd7de Oct 18, 2018

@pchote pchote added this to the Next + 1 milestone Oct 25, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2018

This is a high impact feature, so adding it to the milestone.

@pchote
Copy link
Member

pchote left a comment

This looks great. Just a couple of comments to start with:

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch from 38bd7de to f72f995 Oct 30, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Oct 30, 2018

Here's what it looks like now. Rendering correctly!

image

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 30, 2018

From the screenshot above it looks like you are using the same margins for the TD panel as in the other mods. Note that TD uses its own, smaller, margins because it does not use thick panel borders (see tooltips and the editor sidebar for examples).

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch from f72f995 to 64da9c7 Oct 31, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Oct 31, 2018

Here's the TD selection box made more compact with 3px margins like the actor selector. Let me know if it should be altered.

image

@Phrohdoh

This comment has been minimized.

Copy link
Member

Phrohdoh commented Nov 1, 2018

Would it be possible to also change the actor type?

If not (because of a concern for, for example, size/footprint) then we can always do it in the future. :)

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 3, 2018

There are a few more graphical nits, summarised in this screenshot:

screenshot 2018-11-03 at 15 15 34

  • The Type description can overrun the edge of the dialog. IMO there is no reason to do something special here - it would be better all-round to use the same label as in the selection list. You should then use WidgetUtils.TruncateText to make sure it will fit.
  • The selected faction in the Owner dropdown should be bold to match the dropdowns in the sidebar and elsewhere in the mod (the dropdown panel contents shouldn't change).
  • The save/delete/cancel button labels shouldbe bold to match the rest of the mod.
  • Pressing the enter key in the ID textfield should trigger the save button.

Also:

  • If you select an actor and then try to click on another actor that is in the gap between the first and its edit dialog, the mouse events are blocked. I expect you have a container widget that is blocking the mouse events somewhere.
Show resolved Hide resolved mods/common/chrome/editor.yaml Outdated
@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Nov 6, 2018

Sort of like this as far as the Type name truncation goes? (Edit: No)

image

drogoganor added a commit to drogoganor/OpenRA that referenced this pull request Nov 6, 2018

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Nov 6, 2018

Updated to address issues. I have also made the EditorActorPreview's Tooltip field dynamic with a get property, since the ID and Owner can be set now. This might be a performance concern. And updated the UI:

image

It's in a separate commit but I will squash them if need be.

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Nov 6, 2018

For your consideration, a mockup of how it would look with a bunch of other options:

image

@drogoganor drogoganor force-pushed the drogoganor:map-editor-select branch from 653f797 to ba890da Nov 6, 2018

@pchote pchote force-pushed the drogoganor:map-editor-select branch from ba890da to 66bb606 Nov 18, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 18, 2018

I have pushed over this with fixes for:

  • Selection panel offset (was using the previews full width, instead of width / 2)
  • A crash when actors don't define any tooltip traits

plus rebasing on the latest bleed.

There are a couple of other changes i'd like to investigate before merging.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 18, 2018

I have a slight rework for the user interaction on 13928cb / pchote/map-editor-select. The goal here was to streamline things looking forward to the (hopefully near) future where we expose additional parameters in the panel.

  • Input is now applied immediately instead of losing them if you accidentally click out of the dialog.
  • Cancel button has been replaced by Close, which behaves the same as clicking outside the dialog.
  • Error labels are now shown immediately below the ID field.
  • Widget layout slightly reorganized in preparation for dynamically populated parameters.

If I can get some 👍s on these changes then we can merge them into this PR and then the whole feature into bleed.

I've added the fixup label here for now to make it clear that i'd like to get at least some of these changes here before merging.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 21, 2018

If I can get some 👍s on these changes then we can merge them into this PR and then the whole feature into bleed.

👍 from me.

For those who don't want to/can't compile @pchote's changes, this is how the result looks like:
pic01

@drogoganor

This comment has been minimized.

Copy link
Contributor

drogoganor commented Nov 22, 2018

Hi guys, I apologize for the unresponsiveness. Real life has made demands.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 22, 2018

Hi guys, I apologize for the unresponsiveness. Real life has made demands.

No problem. Just to clarify, For those who don't want to/can't compile @pchote's changes was only directed at potential 3rd-party reviewers, not you.

@pchote pchote force-pushed the drogoganor:map-editor-select branch from 66bb606 to caa591b Nov 24, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 24, 2018

@pchote pchote force-pushed the drogoganor:map-editor-select branch from caa591b to cb22701 Nov 24, 2018

@pchote

pchote approved these changes Nov 24, 2018

@pchote pchote merged commit 22bece2 into OpenRA:bleed Nov 24, 2018

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