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

Editor mirror tiles layer #21317

Merged
merged 1 commit into from Mar 21, 2024

Conversation

drogoganor
Copy link
Contributor

@drogoganor drogoganor commented Jan 27, 2024

Added a mirror tile brush to the map editor for marking out tiles. This should allow creators of mirror maps to have an easier time of things.

There's two modes:

Flip Mode
Flip mirror tiles along one or both central axes of the map.

ra-2024-01-27T040425597Z

You can supply an axis angle to flip along an angled axis. This angle is in increments of 15 degrees.

Rotate Mode
Create mirror tiles by rotating instead of flipping. Can be used for 2-8 player maps.

ra-2024-01-27T040658120Z

Mirror tiles are not persisted to the map file.
The layer is enabled by default in the editor, and is toggled with F3.

Potentially needs a clear button and an editable center offset.

Source artwork can be found here.

@Porenutak
Copy link
Contributor

Porenutak commented Jan 27, 2024

thanks awesome PR again. Here's few notes from user perspective:

  • there should be option to delete mirror tiles or reset selected mirror color.
  • Axis angle should have visual representation. Some kind of neutral line. So user know against what he is mirroring.

@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 6, 2024

This needs to go back into the oven. As I was implementing the axis angle display, I realized the isometric mod tile mirroring only has the broad appearance of working, but is actually completely broken. I need to re-do the mirror tile position calculation in a way that respects the isometric projection.

Additionally, I think I need to put the overlay checkboxes i.e. "Show Terrain Geometry", "Show Buildable Terrain" back into a drop-down at the top of the map like it used to be. Then, a drop-down should be added to the Tools panel that lists other tools such as Mirror Tiles. Upon selecting a tool, the controls for that tool should be displayed in the panel. Otherwise, we're completely out of room already.

@drogoganor drogoganor marked this pull request as draft February 6, 2024 12:53
@drogoganor
Copy link
Contributor Author

Current UI design:
ra-2024-02-08T094519235Z

@drogoganor drogoganor force-pushed the feature/editor-mirror-layer2 branch 4 times, most recently from d19a305 to c880aa5 Compare February 8, 2024 11:36
@drogoganor
Copy link
Contributor Author

drogoganor commented Feb 8, 2024

Still requires some chrome fiddling.
Also all the chrome controls are prefixed with MIRROR_ and variable names are prefixed with mirror which is a bit verbose.
Probably the mirror layer should be enabled when the user starts painting if it's turned off (it's on by default)
The buttons could probably be disabled if there's no tiles to clear.

@drogoganor drogoganor marked this pull request as ready for review February 8, 2024 11:48
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crash

Exception of type `System.IndexOutOfRangeException`: Index was outside the bounds of the array.
   at OpenRA.Mods.Common.Widgets.PaintMirrorTileEditorAction.Add(CPos target) in OpenRA/OpenRA.Mods.Common/EditorBrushes/EditorMirrorLayerBrush.cs:line 138
   at OpenRA.Mods.Common.Widgets.EditorMirrorLayerBrush.HandleMouseInput(MouseInput mi) in /Users/gustas/Documents/GitHub/OpenRA/OpenRA.Mods.Common/EditorBrushes/EditorMirrorLayerBrush.cs:line 76
   at OpenRA.Mods.Common.Widgets.EditorViewportControllerWidget.HandleMouseInput(MouseInput mi) in /OpenRA/OpenRA.Mods.Common/Widgets/EditorViewportControllerWidget.cs:line 89

I feel like there should be an erasor brush added, perhaps add a non-colored square, or with an icon

I also recon that people would like to save and import overlays, atm mappers already do this so there is clearly a need

as for the UI, I feel like the axis guide checkbox should just be to the side of the slider instead of in a separate row. Then the overlays dropdown could be moved there as well

Screenshot 2024-02-08 at 14 23 16

the guide lines could be easily scaled to go to the edges of the map

I'll do a code review later

@PunkPun
Copy link
Member

PunkPun commented Feb 8, 2024

oh, and also for functional purposes I think the overlay should be more transparent. What matters is what is behind the overlay than the overlay itself. I wonder wether we should give control over the transparency to the player 🤔

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be an error with axis calculation in TS

Map: They All Float
Screenshot 2024-02-27 at 14 35 17

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like no mirror mode should be added for completeness. People may want to do corrections

OpenRA.Game/Graphics/MirrorTileRenderable.cs Outdated Show resolved Hide resolved
OpenRA.Game/Graphics/MirrorTileRenderable.cs Outdated Show resolved Hide resolved
@drogoganor
Copy link
Contributor Author

drogoganor commented Mar 3, 2024

Added a not very thrilling Erase icon:
image
Annoying gap between last button and scroll container.
User can now set Mirror Mode to None so they can mark out arbitrary tiles anywhere

Still to fix:

  • Save and load overlays
  • Possibly add axis guide checkbox to the same line as Axis Angle slider

@drogoganor drogoganor force-pushed the feature/editor-mirror-layer2 branch from 46a2045 to f9d2f53 Compare March 3, 2024 11:53
@PunkPun
Copy link
Member

PunkPun commented Mar 3, 2024

related #10513

@drogoganor drogoganor force-pushed the feature/editor-mirror-layer2 branch 2 times, most recently from 5dfd1ef to 8cce1c2 Compare March 4, 2024 09:34
@drogoganor
Copy link
Contributor Author

drogoganor commented Mar 4, 2024

I've added the ability to persist the mirror tiles layer and settings. It automatically saves to [SupportDir]\Editor\{mod}\{version}\MirrorTiles\{mapfilename}.json when you save a map that has any painted mirror tiles. I'm not sure if we like using json but it was definitely convenient.
Not sure if this is what you had in mind but I'm pretty sure it will be suitable for most mapper's needs.

@drogoganor drogoganor force-pushed the feature/editor-mirror-layer2 branch from 17dd308 to bf9e77e Compare March 4, 2024 11:16
Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the PR is in good condition, just some polish is missing. I also want to mention some scope creep

The overlay saving definitely needs to have separate save functionality. The most common usecase is having pre-made overlays that you just import when working on a new map. It means that different map sizes should be handled, centring the overlay when it gets cutoff or is incomplete

It will still be much more polished if the symmetry line was clamped to map edges
Screenshot 2024-03-09 at 22 12 00

OpenRA.Mods.Common/EditorBrushes/EditorMirrorLayerBrush.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/EditorBrushes/EditorMirrorLayerBrush.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/MirrorLayerOverlay.cs Outdated Show resolved Hide resolved
mods/cnc/chrome/editor.yaml Outdated Show resolved Hide resolved
@drogoganor
Copy link
Contributor Author

@Porenutak , you'll need to do a git pull and make all on this branch to get the latest code.

@drogoganor drogoganor force-pushed the feature/editor-mirror-layer2 branch 2 times, most recently from 4286cbb to 2786b7a Compare March 18, 2024 12:39
@Porenutak
Copy link
Contributor

Porenutak commented Mar 18, 2024

Dont know how hard is this to implement, but to speed up workflow it would be nice to switch to eraser via modifier. Like with LMB I wil add mirror tiles, but with CTRL+ LMB I will erase them.

@PunkPun
Copy link
Member

PunkPun commented Mar 18, 2024

I dunno if that would be better. IMO we should keep the current workflow for this PR

@Porenutak
Copy link
Contributor

Its more a thought for the future. Using modifiers is always faster that clicking on tab icons. All modern editors works this way.

@drogoganor
Copy link
Contributor Author

drogoganor commented Mar 18, 2024

We might be fresh out of room on the panel. I discovered when I tried to add controls to save and load the mirror tiles layer, that the scrollbar doesn't enable. I think ScrollPanelWidget doesn't like being pre-populated with child controls from the yaml. Setting the ContentHeight doesn't enable the scrollbar, either. I think it has to be manually populated with ScrollItemWidgets at runtime for scrolling to be enabled.

@PunkPun
Copy link
Member

PunkPun commented Mar 18, 2024

See how the settings panels work

@drogoganor
Copy link
Contributor Author

drogoganor commented Mar 18, 2024

OK yeah, I got that figured out.

ra-2024-03-18T143559390Z

There's a fair bit of work to do on this part so it's foolproof. Needs a validation label like on the Actor ID edit.

A potential concern is that the user could easily overwrite another mirror tile file, since there's no confirmation dialog.
Also argh-worthy is yielding focus from the text input.

@drogoganor
Copy link
Contributor Author

After some consideration, I think it might be better to omit the custom mirror tile file save/load feature, and just automatically persist to a file that matches the map filename instead.
I think the use-case for loading mirror tiles created on other maps is maybe limited. Also I'm worried about users accidentally overwriting a mirror tile file for another map by accident. In general I'm reluctant to have any file management code in the editor other than the map file itself.
Pre-made grids like this:
image
Might have limited utility now since the user can simply paint tiles and have them mirrored as they require.

@PunkPun
Copy link
Member

PunkPun commented Mar 20, 2024

We don't necessarily need ingame file management. But the game should handle imports from maps of other sizes. Currently I get this crash

Exception of type `System.IndexOutOfRangeException`: Index was outside the bounds of the array.
   at OpenRA.CellLayer`1.get_Item(CPos cell) in OpenRA/OpenRA.Game/Map/CellLayer.cs:line 78
   at OpenRA.Graphics.MirrorTileRenderable.Render(WorldRenderer wr) in OpenRA/OpenRA.Game/Graphics/MirrorTileRenderable.cs:line 45
   at OpenRA.Graphics.WorldRenderer.DrawAnnotations() in OpenRA/OpenRA.Game/Graphics/WorldRenderer.cs:line 337

I suppose fixing the TS axis line would be too much of a scope screep. But I wanted to still mention that it's lieing

Screenshot 2024-03-20 at 16 04 53

@Porenutak
Copy link
Contributor

We don't necessarily need ingame file management. But the game should handle imports from maps of other sizes.

Can u give at least one example why anyone needs to import mirror tiles from other map?
I dont see the point.

@drogoganor
Copy link
Contributor Author

drogoganor commented Mar 20, 2024

I think making the mirror tile file work if you manually copy and rename it from another map would be a good idea. I'll get onto that.

Yes, that axis line issue in TS is such a pain. I thought it was due to elevation but apparently not.

@drogoganor
Copy link
Contributor Author

  • I renamed the feature to Marker Tiles instead of Mirror Tiles
  • Now defaults to Mirror Mode: None
  • The "Show Axis Guide" checkbox is now removed (it simply always shows in Flip mode, unless the Marker Tiles overlay is disabled)
  • Renaming a marker tiles file to the name of another map of a different size will no longer cause a crash (tiles that aren't on the map will be dropped)
  • Fixed cnc editor chrome regression

@PunkPun PunkPun merged commit 25a6b4b into OpenRA:bleed Mar 21, 2024
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Mar 21, 2024

changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants