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

Add Actor Categories and Categories dropdown filter in Map editor #13353

Merged
merged 6 commits into from Jul 5, 2017

Conversation

Projects
None yet
7 participants
@rob-v
Contributor

rob-v commented May 25, 2017

Closes #10139

Add Actor categories:
V01:
..EditorTilesetFilter:
....ExcludeTilesets: DESERT
....Categories: Building, Civilian

Add Categories dropdown filter in Map editor:

obrazok

Categories list is combined from all existing categories of all actors.

EditorTilesetFilter can be renamed but due to currently broken Utility, hundreds of changed files (due to renaming or removed comments), I prefer to do it in another PR or at least lately as final commit after review of this PR.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 25, 2017

Member

#13344 fixes the utility.

Member

pchote commented May 25, 2017

#13344 fixes the utility.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 28, 2017

Contributor

This works rather well, the only thing I found a bit inconvenient is that if the list of categories is rather long, toggling off all unwanted categories one by one is rather tedious. Would it be possible to add a "Select all" toggle that can toggle all categories on/off at once?

Contributor

reaperrr commented May 28, 2017

This works rather well, the only thing I found a bit inconvenient is that if the list of categories is rather long, toggling off all unwanted categories one by one is rather tedious. Would it be possible to add a "Select all" toggle that can toggle all categories on/off at once?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 28, 2017

Contributor

Updated - added Select all, Select none options

Contributor

rob-v commented May 28, 2017

Updated - added Select all, Select none options

@MustaphaTR

Works as described. 👍

@pchote

All of the actors for all mods show up in the one "Not category" category. Can you please set up EditorTilesetFilter traits on the defaults in each mod?

Show outdated Hide outdated mods/cnc/chrome/editor.yaml
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 29, 2017

Contributor

@pchote What just crossed my mind, how compatible is this approach to categories wrt translation?

Contributor

reaperrr commented May 29, 2017

@pchote What just crossed my mind, how compatible is this approach to categories wrt translation?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 29, 2017

Member

For that it would be best if we split the internal ID away from the display name shown in the UI.

Member

pchote commented May 29, 2017

For that it would be best if we split the internal ID away from the display name shown in the UI.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 29, 2017

Contributor

Updated - checkboxes replaced by buttons - see new image in PR's description

Contributor

rob-v commented May 29, 2017

Updated - checkboxes replaced by buttons - see new image in PR's description

@obrakmann

lgtm, 👍

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 25, 2017

Contributor

Is there someone having idea/time to set up EditorTilesetFilter traits - Categories on the defaults in each mod? I probably won't work on this next week(s), so I would close it to free PR queue.

Contributor

rob-v commented Jun 25, 2017

Is there someone having idea/time to set up EditorTilesetFilter traits - Categories on the defaults in each mod? I probably won't work on this next week(s), so I would close it to free PR queue.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 25, 2017

Member

That should only be a few minutes work, so it would be a shame to give up on this feature over it.

Member

pchote commented Jun 25, 2017

That should only be a few minutes work, so it would be a shame to give up on this feature over it.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 25, 2017

Contributor

@rob-v @pchote I've set up actor categories locally, I can either push them to this branch or we merge it first and I file a separate PR.

Contributor

reaperrr commented Jun 25, 2017

@rob-v @pchote I've set up actor categories locally, I can either push them to this branch or we merge it first and I file a separate PR.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 25, 2017

Contributor

@reaperrr I talked with @pchote on IRC so was also just finishing it. really not too difficult and results looks useful. IMO if you have it ready, it can be only better than my version, so do as you prefer.

Contributor

rob-v commented Jun 25, 2017

@reaperrr I talked with @pchote on IRC so was also just finishing it. really not too difficult and results looks useful. IMO if you have it ready, it can be only better than my version, so do as you prefer.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 25, 2017

Contributor

Ah sorry, should have checked on IRC first.
If you have your changes ready, I'd say push them first, Travis didn't run on your last update so that way we could also make sure there are no rebase conflicts.

If there is anything in my categories that I consider an improvement, I can still file them in a follow-up.

Contributor

reaperrr commented Jun 25, 2017

Ah sorry, should have checked on IRC first.
If you have your changes ready, I'd say push them first, Travis didn't run on your last update so that way we could also make sure there are no rebase conflicts.

If there is anything in my categories that I consider an improvement, I can still file them in a follow-up.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 25, 2017

Contributor

Thanks for your willigness. I should have rather confirmed here that I'll finish it. Ok, so I'll push it soon.

Contributor

rob-v commented Jun 25, 2017

Thanks for your willigness. I should have rather confirmed here that I'll finish it. Ok, so I'll push it soon.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 25, 2017

Contributor

Updated - added Actor Categories for all mods.

Contributor

rob-v commented Jun 25, 2017

Updated - added Actor Categories for all mods.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 26, 2017

Contributor

Updated - Ore and Gem mines now 'Resource spawn', Huntable changed to Critter.

Contributor

rob-v commented Jun 26, 2017

Updated - Ore and Gem mines now 'Resource spawn', Huntable changed to Critter.

@pchote

I'm really liking this.

Here's some suggestions to polish the category lists:

@@ -827,6 +857,8 @@
ShowOwnerRow: false
FrozenUnderFog:
ScriptTriggers:
EditorTilesetFilter:
Categories: Husk

This comment has been minimized.

@pchote

pchote Jun 27, 2017

Member

I'd personally include this under trees (or maybe both?).

@pchote

pchote Jun 27, 2017

Member

I'd personally include this under trees (or maybe both?).

Show outdated Hide outdated mods/cnc/rules/misc.yaml
Show outdated Hide outdated OpenRA.Mods.Common/Widgets/Logic/Editor/ActorSelectorLogic.cs
Y: 2
Width: 90
Height: 25
Text: Select all

This comment has been minimized.

@pchote

pchote Jun 27, 2017

Member

These both should be Font: Bold

@pchote

pchote Jun 27, 2017

Member

These both should be Font: Bold

Show outdated Hide outdated mods/ts/rules/defaults.yaml
Show outdated Hide outdated mods/ts/rules/misc.yaml
Show outdated Hide outdated mods/ts/rules/defaults.yaml
Show outdated Hide outdated mods/ra/rules/misc.yaml
Show outdated Hide outdated mods/ra/rules/defaults.yaml
@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

RA Bridge has empty category, no actors. Remove it from .yaml or should be fixed?
RA has 2 'Resource spawn' actors, TD and TS none. Should we move 'Blossom Tree's to 'Resource spawn' or change all to 'System'?

Contributor

rob-v commented Jun 28, 2017

RA Bridge has empty category, no actors. Remove it from .yaml or should be fixed?
RA has 2 'Resource spawn' actors, TD and TS none. Should we move 'Blossom Tree's to 'Resource spawn' or change all to 'System'?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 28, 2017

Member

RA and TD bridges are currently autogenerated by the map, and shouldn't be placed manually. This will eventually change once they are rewritten to use the new/better TS bridge logic, but we can restore those categories when that happens. I think having a separate Resource Spawn category for all mods is a good idea. In TS this should include vein holes.

Member

pchote commented Jun 28, 2017

RA and TD bridges are currently autogenerated by the map, and shouldn't be placed manually. This will eventually change once they are rewritten to use the new/better TS bridge logic, but we can restore those categories when that happens. I think having a separate Resource Spawn category for all mods is a good idea. In TS this should include vein holes.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

Updated.

Contributor

rob-v commented Jun 28, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 28, 2017

Contributor

In both RA and TD, in my opinion CAMERA should move to System, and FLARE, mpspawn and waypoint should be added to that, too.

Also, in RA I'd move QUEE and LARVA(E) to Critter and CTFLAG to System.

👍 once that and removing the redundant TS bridge entries is done.

Contributor

reaperrr commented Jun 28, 2017

In both RA and TD, in my opinion CAMERA should move to System, and FLARE, mpspawn and waypoint should be added to that, too.

Also, in RA I'd move QUEE and LARVA(E) to Critter and CTFLAG to System.

👍 once that and removing the redundant TS bridge entries is done.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 28, 2017

Member

@reaperrr, @rob-v: would you object to adding this and #13337 to the playtest milestone (and dealing with reviews and fixups on the associated deadline)? It would be nice to be able to add a section on map editor improvements to our playtest annoucement news post.

Member

pchote commented Jun 28, 2017

@reaperrr, @rob-v: would you object to adding this and #13337 to the playtest milestone (and dealing with reviews and fixups on the associated deadline)? It would be nice to be able to add a section on map editor improvements to our playtest annoucement news post.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 28, 2017

Contributor

This is already done as was requested also by @pchote. or not?

Sorry, yes, seems I had forgotten to fetch the latest version of the branch. Only the TS bridges need to be updated then.

Contributor

reaperrr commented Jun 28, 2017

This is already done as was requested also by @pchote. or not?

Sorry, yes, seems I had forgotten to fetch the latest version of the branch. Only the TS bridges need to be updated then.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

Fixing, please wait with review.

Contributor

rob-v commented Jun 28, 2017

Fixing, please wait with review.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

Also, in RA I'd move QUEE and LARVA(E) to Critter and CTFLAG to System.

QUEE Queen Ant has Building trait? I didn't add Category, because it isn't visible in Map editor. @pchote proposed now in IRC to include in Map editor only actors with EditorTilesetFilter (EditorData) trait. Adding Critter could be done, because it probably still won't be displayed for other reasons (as it isn't also now). However this would be inconsistent as I shouldn't add new Category for not visible actors, because such empty category would be in the list.

CTFLAG - is currently Tech building. I can change category, but I think it isn't displayed anyway, so not needed?

Contributor

rob-v commented Jun 28, 2017

Also, in RA I'd move QUEE and LARVA(E) to Critter and CTFLAG to System.

QUEE Queen Ant has Building trait? I didn't add Category, because it isn't visible in Map editor. @pchote proposed now in IRC to include in Map editor only actors with EditorTilesetFilter (EditorData) trait. Adding Critter could be done, because it probably still won't be displayed for other reasons (as it isn't also now). However this would be inconsistent as I shouldn't add new Category for not visible actors, because such empty category would be in the list.

CTFLAG - is currently Tech building. I can change category, but I think it isn't displayed anyway, so not needed?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

I didn't see QUEE, because it is visible only in INTERIOR tileset, so I'll add it to Critter.

Contributor

rob-v commented Jun 28, 2017

I didn't see QUEE, because it is visible only in INTERIOR tileset, so I'll add it to Critter.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 28, 2017

Contributor

Updated Categories as requested, removed No category. To display actor in Map editor, actor must have assigned EditorTilesetFilter with non empty Categories.

Contributor

rob-v commented Jun 28, 2017

Updated Categories as requested, removed No category. To display actor in Map editor, actor must have assigned EditorTilesetFilter with non empty Categories.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 29, 2017

Contributor

@pchote are you fine with the last changes?

Contributor

reaperrr commented Jun 29, 2017

@pchote are you fine with the last changes?

@reaperrr reaperrr added this to the Playtest featuring updated HitShapes milestone Jun 29, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jun 30, 2017

Member

Categories now look much better. Here are a few final polish thoughts:

  • D2K I'm not sure if it's worth having the frigate, ornithoper accessible in the editor. They won't work properly when placed on a map.
  • D2K having a wall category isn't really worth it (but keep it in the other mods).
  • TS hospital, light tower, light posts should move from Building to Civilian Building.
  • TS Deployed ICBM probably wants to move somewhere other than Building.
  • TS maybe add a dedicated Billboard category? There sure are a lot of those.
  • TS maybe merge Gates into Wall?
  • TS we seem to be missing all the nod vehicles?
  • TD i'm not sure if it's worth having the supply aircraft, a10 accessible in the editor. They won't work properly when placed on a map.
  • TD you have a separate Husk category that contains civilian building husks, but then tree husks are in with the trees. Please pick a convention (IMO having the civ husks with civs and tree husks with trees would be better, since these are legitimate map props. "Normal" player-unit husks are temporary actors that probably shouldn't be exposed in the editor in any mod).
  • RA i'm not sure if it's worth having the spy plane, or badger variants accessible in the editor. They won't work properly when placed on a map.
  • RA maybe merge Gates into Wall?
  • RA explosive barrels and ammo boxes are in with tech buildings?
Member

pchote commented Jun 30, 2017

Categories now look much better. Here are a few final polish thoughts:

  • D2K I'm not sure if it's worth having the frigate, ornithoper accessible in the editor. They won't work properly when placed on a map.
  • D2K having a wall category isn't really worth it (but keep it in the other mods).
  • TS hospital, light tower, light posts should move from Building to Civilian Building.
  • TS Deployed ICBM probably wants to move somewhere other than Building.
  • TS maybe add a dedicated Billboard category? There sure are a lot of those.
  • TS maybe merge Gates into Wall?
  • TS we seem to be missing all the nod vehicles?
  • TD i'm not sure if it's worth having the supply aircraft, a10 accessible in the editor. They won't work properly when placed on a map.
  • TD you have a separate Husk category that contains civilian building husks, but then tree husks are in with the trees. Please pick a convention (IMO having the civ husks with civs and tree husks with trees would be better, since these are legitimate map props. "Normal" player-unit husks are temporary actors that probably shouldn't be exposed in the editor in any mod).
  • RA i'm not sure if it's worth having the spy plane, or badger variants accessible in the editor. They won't work properly when placed on a map.
  • RA maybe merge Gates into Wall?
  • RA explosive barrels and ammo boxes are in with tech buildings?

@pchote pchote dismissed their stale review Jun 30, 2017

Blocking changes have been fixed.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 30, 2017

Contributor
D2K Should I remove frigate, ornithoper?
D2K Wall moved to Defense.
TS These actors don't inherit from ^CivBuilding like most others in that .yaml, so added category Civilian building.
TS Deployed ICBM is in civilian-structures.yaml, so also Civilian building?
TS Billboards moved to new Billboard category.
TS Gates moved to Wall.
TS Nod vehicles are missing also on bleed, though they are added to the panel, any clue?
TD Should I remove supply aircraft, a10?
TD I had husks in Husk category, TreeHusk was moved to Tree on your request.
RA Should I remove spy plane or badger variants?
RA Gates moved to Wall.
RA Explosive Barrel and Ammo Box inherit from TechBuilding. To which category they should be moved?

Re Aircraft 'They won't work properly when placed on a map.'. If we are not sure, I propose to keep it, it is safer. Next PR can polish categories instead of having to revert it as missing in Map editor due to this PR.
These latest changes are in one commit. After final review I can squash commits if needed.

Contributor

rob-v commented Jun 30, 2017

D2K Should I remove frigate, ornithoper?
D2K Wall moved to Defense.
TS These actors don't inherit from ^CivBuilding like most others in that .yaml, so added category Civilian building.
TS Deployed ICBM is in civilian-structures.yaml, so also Civilian building?
TS Billboards moved to new Billboard category.
TS Gates moved to Wall.
TS Nod vehicles are missing also on bleed, though they are added to the panel, any clue?
TD Should I remove supply aircraft, a10?
TD I had husks in Husk category, TreeHusk was moved to Tree on your request.
RA Should I remove spy plane or badger variants?
RA Gates moved to Wall.
RA Explosive Barrel and Ammo Box inherit from TechBuilding. To which category they should be moved?

Re Aircraft 'They won't work properly when placed on a map.'. If we are not sure, I propose to keep it, it is safer. Next PR can polish categories instead of having to revert it as missing in Map editor due to this PR.
These latest changes are in one commit. After final review I can squash commits if needed.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 4, 2017

Member

For the sake of not dragging out this PR even more, I think it is fine to not remove the aircraft actors.

Member

pchote commented Jul 4, 2017

For the sake of not dragging out this PR even more, I think it is fine to not remove the aircraft actors.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 4, 2017

Member

TS Deployed ICBM should be in the same category as the normal undeployed ICBM.

Member

pchote commented Jul 4, 2017

TS Deployed ICBM should be in the same category as the normal undeployed ICBM.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 4, 2017

Member

RA Explosive Barrel and Ammo Box inherit from TechBuilding. To which category they should be moved?

I would put these, the things currently in "Rock" (which includes tank traps on temperate maps), and the boxes / power poles / ice floes that are currently in "Tree" into a "Decoration" category.

Member

pchote commented Jul 4, 2017

RA Explosive Barrel and Ammo Box inherit from TechBuilding. To which category they should be moved?

I would put these, the things currently in "Rock" (which includes tank traps on temperate maps), and the boxes / power poles / ice floes that are currently in "Tree" into a "Decoration" category.

@cjshmyr

This comment has been minimized.

Show comment
Hide comment
@cjshmyr

cjshmyr Jul 4, 2017

Member

TS we seem to be missing all the nod vehicles?

Possibly #13492

Member

cjshmyr commented Jul 4, 2017

TS we seem to be missing all the nod vehicles?

Possibly #13492

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 5, 2017

Member

Possibly #13492

This indeed appears to be the case, so we can ignore that here.

Member

pchote commented Jul 5, 2017

Possibly #13492

This indeed appears to be the case, so we can ignore that here.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jul 5, 2017

Contributor

RA/CNC/TS - Rocks moved to Decoration
RA Explosive Barrel, Ice Floes, Ammo Box moved to Decoration
TS Deployed ICBM moved to Vehicle

Contributor

rob-v commented Jul 5, 2017

RA/CNC/TS - Rocks moved to Decoration
RA Explosive Barrel, Ice Floes, Ammo Box moved to Decoration
TS Deployed ICBM moved to Vehicle

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 5, 2017

Contributor

My 👍 still stands.

Contributor

reaperrr commented Jul 5, 2017

My 👍 still stands.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 5, 2017

Member

To save time I have pushed fixes for a couple remaining nits here:

  • RA Moved barl from Tech building to Decoration.
  • RA moved ctflag and flare from System to Decoration.
  • RA moved boxes01-09 and utilpol1-2 from Tree to Decoration.
Member

pchote commented Jul 5, 2017

To save time I have pushed fixes for a couple remaining nits here:

  • RA Moved barl from Tech building to Decoration.
  • RA moved ctflag and flare from System to Decoration.
  • RA moved boxes01-09 and utilpol1-2 from Tree to Decoration.
@pchote

pchote approved these changes Jul 5, 2017

👍 with my final fixes (pushed here).

@reaperrr if you are happy with those changes then please go ahead and merge this.

@reaperrr reaperrr merged commit 1a546d9 into OpenRA:bleed Jul 5, 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