Page MenuHomeWildfire Games

Atlas: Allow map to recenter during resize
Needs ReviewPublic

Authored by Stan on Aug 22 2017, 1:10 AM.

Details

Summary

Allows maps to have the center changed during resizing.
For maps being shrunk, this allows the desired, remaining, portion of the map to be chosen:

For maps being enlarged, this allows the user to choose where the original map data should be placed:

Test Plan

Use provided maps to verify that function chooses the correct location and decal/unit mapping.
(should be placed in binaries/data/mods/public/maps/scenarios)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8050
Build 13101: Vulcan BuildJenkins

Event Timeline

Clockwork-Muse created this revision.Aug 22 2017, 1:10 AM
Stan added a reviewer: trompetin17.
vladislavbelov requested changes to this revision.Aug 24 2017, 9:00 PM

I've compiled and tested it. The tool is pretty useful.

  • Resize tool looks weird in the Terrain tab, I suggest to move it to the Map, because the Terrain tab is for the terrain modification.
  • The minimap mask looks like Win98, it's possible make just black with alpha channel instead of these dots, which aren't transparent.
  • If the map is resized, then borders are repeated, probably we need to fill new areas with the default texture?
  • Bump years at the license part on the top of files.
  • I see duplications of some functions, could we reuse some code?
  • There are few magic constants, we should avoid it.

P.S. Please, use a command like svn diff --diff-cmd diff -x "-U 99999" > changes.patch to create patches with its context. Because currently Phabricator shows the Context not available. message, so we're not able to look at the code around.

source/graphics/Terrain.cpp
504

One line isn't so long.

570

Brackets on the new line (below too), the file should contain the one style.

source/tools/atlas/AtlasUI/CustomControls/MapResizeDialog/PsuedoMiniMapPanel.cpp
26 ↗(On Diff #3277)

Do not use C headers instead of C++ headers. Also some people prefer to add new line, because it's the standard header and not the wxWidget one.

51 ↗(On Diff #3277)

It looks like the duplication of the code, there is the same function below. I think, we have to avoid this.

158 ↗(On Diff #3277)

I think, we should handle mouse leave event too, if it's possible. Because it gives a strange behaviour.

225 ↗(On Diff #3277)

Indentation, and below.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
21

Use the alphabet order.

324

As I said below, raw pointers isn't the good practice for threaded applications. I'd prefer to use std::vector<u8> instead of this C-style.

378

Useless brackets.

392

Why void* and not u8?

562

The auto isn't preferred for this cases, because the name or the type doesn't tell about itself.

610

Won't entities be re-added?

source/tools/atlas/GameInterface/Messages.h
205

It's not good to pass raw pointers, especially in threaded applications. There are few places with raw pointers in the file. But we shouldn't follow the bad practice.

This revision now requires changes to proceed.Aug 24 2017, 9:00 PM
Clockwork-Muse marked 11 inline comments as done.Sep 3 2017, 5:24 AM
  • I see duplications of some functions, could we reuse some code?

Did you have a specific function(s) in mind?

  • There are few magic constants, we should avoid it.

I dislike them too! What did I miss, please?

source/tools/atlas/AtlasUI/CustomControls/MapResizeDialog/PsuedoMiniMapPanel.cpp
51 ↗(On Diff #3277)

The only other Within I know about is in MapHandler, which, while checking for the same thing overall, is referencing different types. And is in completely separate projects. I could switch to raw x/y values, I guess, but where would you expect the function to go?

158 ↗(On Diff #3277)

Did you have a specific example? Or you mean as a "just in case"?

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
392

Because previously I was directly passing it into the destination wxImage, so just was working on the exact type I needed.

610

I don't understand your comment? What are you getting at here?

Clockwork-Muse marked 2 inline comments as done.

Current texture behavior has been left unchanged. It's unclear how much of the default texture could be left anyways, but this also helps with underwater textures. Can revisit if needed.

Map now uses just a simple 50% (I think...?) transparency. I'm not completely sure why I initially chose the stipple effect (unless I just was so used to the "90's look", indeed), but it does look much better now:

Any news on this project?

vladislavbelov added a comment.EditedFeb 22 2018, 7:19 PM
In D825#54178, @asterix wrote:

Any news on this project?

I think, I should test it again. And then commit, if there won't be a problem.

In D825#54178, @asterix wrote:

Any news on this project?

I think, I should test it again. And then commit, if there won't be a problem.

Thank you very much, because this very useful feature want a lot of people and it will be very useful for mod called Hyrule Conquest and others.

Stan added a subscriber: Stan.May 3 2019, 9:25 AM

@vladislavbelov ping. Can you review it ?

Stan commandeered this revision.Sat, Jun 22, 2:40 PM
Stan added a reviewer: Clockwork-Muse.

Commandeering to rebase @Clockwork-Muse feel free to commandeer back if you want to continue working on it.

Stan updated this revision to Diff 8578.Sat, Jun 22, 2:40 PM
  • Whitespace
  • Static_cast
  • spaces to tabs
  • rebase
Stan marked 2 inline comments as done and an inline comment as not done.Sat, Jun 22, 2:41 PM
Stan marked 2 inline comments as done.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.h
|   1| /*·Copyright·(C)·2012·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2012"

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ((std::vector<std::string>,·data))
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
| 211| »   »   &g_Renderer.GetPostprocManager(),
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1791/display/redirect

Stan updated this revision to Diff 8585.Sun, Jun 23, 6:01 PM

Try to fix the build on Linux

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/AtlasUI/ScenarioEditor/Sections/Terrain/Terrain.h
|   1| /*·Copyright·(C)·2012·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2012"

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ((std::vector<std::string>,·data))
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/MapHandlers.cpp
| 211| »   »   &g_Renderer.GetPostprocManager(),
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1797/display/redirect