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 missing tiles to arrakis.yaml and fixed the map import #12784

Merged
merged 3 commits into from Mar 18, 2017

Conversation

Projects
None yet
6 participants
@abcdefg30
Member

abcdefg30 commented Feb 15, 2017

My take on #12244.
Fixes #12202.

This was referenced Feb 15, 2017

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 16, 2017

Contributor

All right, finaly someone with the "right" approach :)

Contributor

CH4Code commented Feb 16, 2017

All right, finaly someone with the "right" approach :)

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 17, 2017

Contributor
Error 1

Map: 2PLAY13 - Seitch Shuloch
Tileset: Bloxwast.r8
EDIT: Tileset: Bloxxmas.r8

Original OpenRA
image image

The tile id from the right picture is 65 but I didn't found the correct tile in OpenRA. The Assetbrowser showed me that the sequences for the correct (left) tile are 289, 290, 309, 310 in bloxxmas.r8 and this tile is missing in the mods/d2k/tilesets/arrakis.yaml file

Error 2

Map: 4PLAY10 - Tenaya
Tileset: Bloxxmas.r8

Well, there is always one who isn't like the others... :) (OpenRA)
image
Original
image
Tile ID map
tileidmap

If I understand correctly the sequence ids of the tile ids are corresponding to the different tilesets and result in these erros but I think you already know that :D

Error 3

Map: 6PLAY3 - Mauddib's Cave
Tileset: Bloxwast.r8

Original OpenRA
image image
wrong tile is part of 101 -> should be 80
image image
wrong tile is part of 220 should be 250 or 350, they look the same :S

Note

I checked all the multiplayer maps but they really lack of tree/ice/wast tileset maps. Maybe I find more in the mission map pool. There is enough material in the mission pool, I'll try to test them tomorrow.

Contributor

ltem commented Feb 17, 2017

Error 1

Map: 2PLAY13 - Seitch Shuloch
Tileset: Bloxwast.r8
EDIT: Tileset: Bloxxmas.r8

Original OpenRA
image image

The tile id from the right picture is 65 but I didn't found the correct tile in OpenRA. The Assetbrowser showed me that the sequences for the correct (left) tile are 289, 290, 309, 310 in bloxxmas.r8 and this tile is missing in the mods/d2k/tilesets/arrakis.yaml file

Error 2

Map: 4PLAY10 - Tenaya
Tileset: Bloxxmas.r8

Well, there is always one who isn't like the others... :) (OpenRA)
image
Original
image
Tile ID map
tileidmap

If I understand correctly the sequence ids of the tile ids are corresponding to the different tilesets and result in these erros but I think you already know that :D

Error 3

Map: 6PLAY3 - Mauddib's Cave
Tileset: Bloxwast.r8

Original OpenRA
image image
wrong tile is part of 101 -> should be 80
image image
wrong tile is part of 220 should be 250 or 350, they look the same :S

Note

I checked all the multiplayer maps but they really lack of tree/ice/wast tileset maps. Maybe I find more in the mission map pool. There is enough material in the mission pool, I'll try to test them tomorrow.

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 19, 2017

Contributor

So I finally tested the mission maps now and I found only two tile errors :)

Map: O7V1
Tileset: bloxwast

Original OpenRA
image image
conclusion

Additional notes:
I don't know if the following points are in scope of the importer but I stumbled over them during the review process

  • Note 1: Some buildings (Corrino?) won't be imported correctly.
Original OpenRA
image image
image image
  • Note 2: If a map has some kind of "padding" the importer will crash
scali
Map with black padding
$ ./OpenRA.Utility.exe d2k --import-d2k-map missionMaps/A2V2.MAP bloxtree.r8
System.IndexOutOfRangeException: Array index is out of range.
  at OpenRA.CellLayer`1[OpenRA.TerrainTile].set_Item (CPos cell, TerrainTile value) [0x00000] in <filename unknown>:0 
  at OpenRA.Mods.D2k.UtilityCommands.D2kMapImporter.FillMap () [0x00000] in <filename unknown>:0 
  at OpenRA.Mods.D2k.UtilityCommands.D2kMapImporter..ctor (System.String filename, System.String tileset, OpenRA.Ruleset rules) [0x00000] in <filename unknown>:0

@pchote mentioned that a fix for such maps, could cause bogus imports for others.
This only effects the first 4 Missions of every faction (A1V1, A1V2, A2V1,....), which are already imported by @CH4Code (https://github.com/CH4Code/D2k-Campaign-Resources), so ¯\_(ツ)_/¯

Contributor

ltem commented Feb 19, 2017

So I finally tested the mission maps now and I found only two tile errors :)

Map: O7V1
Tileset: bloxwast

Original OpenRA
image image
conclusion

Additional notes:
I don't know if the following points are in scope of the importer but I stumbled over them during the review process

  • Note 1: Some buildings (Corrino?) won't be imported correctly.
Original OpenRA
image image
image image
  • Note 2: If a map has some kind of "padding" the importer will crash
scali
Map with black padding
$ ./OpenRA.Utility.exe d2k --import-d2k-map missionMaps/A2V2.MAP bloxtree.r8
System.IndexOutOfRangeException: Array index is out of range.
  at OpenRA.CellLayer`1[OpenRA.TerrainTile].set_Item (CPos cell, TerrainTile value) [0x00000] in <filename unknown>:0 
  at OpenRA.Mods.D2k.UtilityCommands.D2kMapImporter.FillMap () [0x00000] in <filename unknown>:0 
  at OpenRA.Mods.D2k.UtilityCommands.D2kMapImporter..ctor (System.String filename, System.String tileset, OpenRA.Ruleset rules) [0x00000] in <filename unknown>:0

@pchote mentioned that a fix for such maps, could cause bogus imports for others.
This only effects the first 4 Missions of every faction (A1V1, A1V2, A2V1,....), which are already imported by @CH4Code (https://github.com/CH4Code/D2k-Campaign-Resources), so ¯\_(ツ)_/¯

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Feb 19, 2017

Member

Side specific structure images in OpenRA are set according to side itself. If side is Harkonnen and not Corrino it is normal that it use Harkonnen Palace. Importer doesn't create players afaik, unless this PR added so.

Corrino, Mercenaries, Fremen and Smugglers are valid sides on OpenRA.

Member

MustaphaTR commented Feb 19, 2017

Side specific structure images in OpenRA are set according to side itself. If side is Harkonnen and not Corrino it is normal that it use Harkonnen Palace. Importer doesn't create players afaik, unless this PR added so.

Corrino, Mercenaries, Fremen and Smugglers are valid sides on OpenRA.

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 19, 2017

Contributor

I originaly didn't check the multiplayer maps for tile errors, since those were allready (manually) fixed and done (in the official map pool). So abcdefg30's corrected importer based on my (not meant to be overarching) fixes should only work 100% in case of missions.

@item: The 1st 4 missions in my repo should not all display correctly in vanilla OpenRA, since they use a custom tileset (arrakis.yaml). Whould be surprised if they did.

Contributor

CH4Code commented Feb 19, 2017

I originaly didn't check the multiplayer maps for tile errors, since those were allready (manually) fixed and done (in the official map pool). So abcdefg30's corrected importer based on my (not meant to be overarching) fixes should only work 100% in case of missions.

@item: The 1st 4 missions in my repo should not all display correctly in vanilla OpenRA, since they use a custom tileset (arrakis.yaml). Whould be surprised if they did.

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 19, 2017

Contributor

I could extend the custom tileset, which abcdefg30 then could translate into c#/map importer, fixing all (original) multiplayer maps. But I will only do this if it is requested by abcdefg30 or pchote. It's a ton of work.

Contributor

CH4Code commented Feb 19, 2017

I could extend the custom tileset, which abcdefg30 then could translate into c#/map importer, fixing all (original) multiplayer maps. But I will only do this if it is requested by abcdefg30 or pchote. It's a ton of work.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Feb 20, 2017

Member

Those black area in maps added on Gruntmods edition and doesn't exist in either original 1.2 or 1.6. It is added for Hi-Res Patch or they was crashing.

Member

MustaphaTR commented Feb 20, 2017

Those black area in maps added on Gruntmods edition and doesn't exist in either original 1.2 or 1.6. It is added for Hi-Res Patch or they was crashing.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 20, 2017

Member

@ltem thanks for going through all the missions again.

So as far as I understand, this only seems to fix the tileset for missions.
What about taking this (after I fixed O7V1) and adding the required new tiles as needed when importing the multiplayer maps? (So this PR doesn't get any larger. It wouldn't fix #12202 anymore then, however.)

Member

abcdefg30 commented Feb 20, 2017

@ltem thanks for going through all the missions again.

So as far as I understand, this only seems to fix the tileset for missions.
What about taking this (after I fixed O7V1) and adding the required new tiles as needed when importing the multiplayer maps? (So this PR doesn't get any larger. It wouldn't fix #12202 anymore then, however.)

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 20, 2017

Contributor

@abcdefg30 seems fine, #12788 is for multiplayer maps and could have it's own pull request. Having the converter fixed for Missions will greatly benefit the campaign ticket (#9287).

Contributor

CH4Code commented Feb 20, 2017

@abcdefg30 seems fine, #12788 is for multiplayer maps and could have it's own pull request. Having the converter fixed for Missions will greatly benefit the campaign ticket (#9287).

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 20, 2017

Contributor

@abcdefg30 I'm totally fine with that

Most of the multiplayer tile errors seem to be caused by bloxxmas, which wasn't used for the missions, so it seems fitting to fix this in an other PR.

Contributor

ltem commented Feb 20, 2017

@abcdefg30 I'm totally fine with that

Most of the multiplayer tile errors seem to be caused by bloxxmas, which wasn't used for the missions, so it seems fitting to fix this in an other PR.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 20, 2017

Member

Updated and fixed the remaining two tiles.

Member

abcdefg30 commented Feb 20, 2017

Updated and fixed the remaining two tiles.

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 20, 2017

Contributor

O7V1 is fixed so 👍 from me

Contributor

ltem commented Feb 20, 2017

O7V1 is fixed so 👍 from me

@ltem

ltem approved these changes Feb 20, 2017

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 21, 2017

Contributor

Hi, went through the bugs in the MP maps as well.(Thanks to Item) Those additional tiles (no duplicates) will fix importing all MP maps.
Also I added a "Bridge" Category for all the bridge tiles. On 4P10, 6P4, 6P5 however, the bridges are partially only passable by infantry(due to other predefined tiles) - but manual fix in like 1 minute with the new tiles.
@abcdefg30 just add "Bridge" category and add these lines to your arrrakis.yaml:
http://pastebin.com/C6xzM4xZ (valid for one week)
This will finalize our map-import-effort I hope :)

Contributor

CH4Code commented Feb 21, 2017

Hi, went through the bugs in the MP maps as well.(Thanks to Item) Those additional tiles (no duplicates) will fix importing all MP maps.
Also I added a "Bridge" Category for all the bridge tiles. On 4P10, 6P4, 6P5 however, the bridges are partially only passable by infantry(due to other predefined tiles) - but manual fix in like 1 minute with the new tiles.
@abcdefg30 just add "Bridge" category and add these lines to your arrrakis.yaml:
http://pastebin.com/C6xzM4xZ (valid for one week)
This will finalize our map-import-effort I hope :)

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 22, 2017

Member

Thanks. Added it.

Member

abcdefg30 commented Feb 22, 2017

Thanks. Added it.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Feb 27, 2017

Contributor

In addition to the category fix, the destroyed north/south bridge ends are currently missing.

I've created yaml definitions here:
https://gist.github.com/reaperrr/054c08bb66e6cbd8fda889e3d434d521

@item @CH4Code can you confirm that they look as they should and have correct (im)passability?

brdg1
brdg2

Contributor

reaperrr commented Feb 27, 2017

In addition to the category fix, the destroyed north/south bridge ends are currently missing.

I've created yaml definitions here:
https://gist.github.com/reaperrr/054c08bb66e6cbd8fda889e3d434d521

@item @CH4Code can you confirm that they look as they should and have correct (im)passability?

brdg1
brdg2

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 27, 2017

Contributor

In addition to the category fix, the destroyed north/south bridge ends are currently missing.

Yes and no. Look at Template 513 and 514 in rock detail (where the rest of the bridges are). If you copy/paste parts of it you can construct the big destroyed north/south ends. Thats why the map is generated correctly without them "missing", as is.

The openRA devs originaly simply misinterpreted the BLOX* tile file, i guess (which is't hard at all :D ). There, the bridges are dispalyed in a "compressed" format (i.e. only small bridges).

can you confirm that they look as they should and have correct (im)passability?

They are nor passable at all they way you defined them. Judging from 513 and 514 the centermost bridge tile should be "Rough"(passable for inf), the other "Cliff".

@reaperrr I don't think it is needed to fix importing, though.
Of course detatched ends have better useability in map editor :)

Contributor

CH4Code commented Feb 27, 2017

In addition to the category fix, the destroyed north/south bridge ends are currently missing.

Yes and no. Look at Template 513 and 514 in rock detail (where the rest of the bridges are). If you copy/paste parts of it you can construct the big destroyed north/south ends. Thats why the map is generated correctly without them "missing", as is.

The openRA devs originaly simply misinterpreted the BLOX* tile file, i guess (which is't hard at all :D ). There, the bridges are dispalyed in a "compressed" format (i.e. only small bridges).

can you confirm that they look as they should and have correct (im)passability?

They are nor passable at all they way you defined them. Judging from 513 and 514 the centermost bridge tile should be "Rough"(passable for inf), the other "Cliff".

@reaperrr I don't think it is needed to fix importing, though.
Of course detatched ends have better useability in map editor :)

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 27, 2017

Contributor

Uhh, I derped a bit, I should have seen that.

In the original tileset, the bridges are smaller. Edit: Yeah "compressed" sounds very good. I don't know what is to prefer for map makers.
image
Source: http://old.d2kplus.com/wiki/index.php?title=File:Bloxxmas.png

Infantry can pass the bridge "slopes" (but not vehicles), the cursor gets grey so it should be Rough
image
image

I would move the tiles 513, 514, 515 to the Bridge category as well .

Contributor

ltem commented Feb 27, 2017

Uhh, I derped a bit, I should have seen that.

In the original tileset, the bridges are smaller. Edit: Yeah "compressed" sounds very good. I don't know what is to prefer for map makers.
image
Source: http://old.d2kplus.com/wiki/index.php?title=File:Bloxxmas.png

Infantry can pass the bridge "slopes" (but not vehicles), the cursor gets grey so it should be Rough
image
image

I would move the tiles 513, 514, 515 to the Bridge category as well .

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 27, 2017

Contributor

Yeah "compressed" sounds very good. I don't know what is to prefer for map makers.

Smaller, but identifyable "building blocks" imho. But, if we remove/reorganize existing tiles alltogether, that surely will cause problems for existing maps.

Contributor

CH4Code commented Feb 27, 2017

Yeah "compressed" sounds very good. I don't know what is to prefer for map makers.

Smaller, but identifyable "building blocks" imho. But, if we remove/reorganize existing tiles alltogether, that surely will cause problems for existing maps.

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Feb 27, 2017

Contributor

Nice :) Found the last fix :) Replace
513 4: Rough -> 4: Transition and
514 4: Rough -> 4: Transition.
EDIT:
Also 515 needs fixing, or the units can leave the bridge mid-way:
grafik
515 1: Sand -> 1: Cliff
2: Sand -> 2: Cliff
Then all passability issues on 4PLAY10 are auto-fixed :)

Contributor

CH4Code commented Feb 27, 2017

Nice :) Found the last fix :) Replace
513 4: Rough -> 4: Transition and
514 4: Rough -> 4: Transition.
EDIT:
Also 515 needs fixing, or the units can leave the bridge mid-way:
grafik
515 1: Sand -> 1: Cliff
2: Sand -> 2: Cliff
Then all passability issues on 4PLAY10 are auto-fixed :)

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Feb 27, 2017

Contributor

I agree with you about the smaller building blocks and as you already said before (^^ I tried to 👍 it up but it was already gone), this should/could be done in an other PR.

Contributor

ltem commented Feb 27, 2017

I agree with you about the smaller building blocks and as you already said before (^^ I tried to 👍 it up but it was already gone), this should/could be done in an other PR.

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Mar 1, 2017

Contributor

Also ..... just some cosmetics : could you moveTemplate@1108: to be before Template@1085:?
Has nice (originally intended, messed up ^^) effect on tile order in editor:
grafik

Contributor

CH4Code commented Mar 1, 2017

Also ..... just some cosmetics : could you moveTemplate@1108: to be before Template@1085:?
Has nice (originally intended, messed up ^^) effect on tile order in editor:
grafik

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 4, 2017

Member

Sorry for the delay, updated.

Member

abcdefg30 commented Mar 4, 2017

Sorry for the delay, updated.

@ltem

This comment has been minimized.

Show comment
Hide comment
@ltem

ltem Mar 5, 2017

Contributor

The end is near :D

  • I went through all the multiplayer maps and the only tile error if found is Error 3 of my comment #12784 (comment). It still exits (Map: 6PLAY3.MAP.zip Tileset: bloxwast.r8)

  • I will look into the passability issues in the follow up PR

So after Error 3 is fixed 👍 from me again

Contributor

ltem commented Mar 5, 2017

The end is near :D

  • I went through all the multiplayer maps and the only tile error if found is Error 3 of my comment #12784 (comment). It still exits (Map: 6PLAY3.MAP.zip Tileset: bloxwast.r8)

  • I will look into the passability issues in the follow up PR

So after Error 3 is fixed 👍 from me again

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Mar 5, 2017

Contributor

Yep, those two errors are C# solvable only. 👍, too, after this :)
EDIT: Btw. my review changes (passability) still need to be typed into the file. Only four small changes ;)

Contributor

CH4Code commented Mar 5, 2017

Yep, those two errors are C# solvable only. 👍, too, after this :)
EDIT: Btw. my review changes (passability) still need to be typed into the file. Only four small changes ;)

@ltem ltem referenced this pull request Mar 6, 2017

Merged

D2K - Add Ordos Mission 2a #12880

@pchote pchote added this to the Next + 1 milestone Mar 14, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 15, 2017

Member

Sorry for the late update. Finally fixed it. This should be good to go now.

Member

abcdefg30 commented Mar 15, 2017

Sorry for the late update. Finally fixed it. This should be good to go now.

@CH4Code

This comment has been minimized.

Show comment
Hide comment
@CH4Code

CH4Code Mar 16, 2017

Contributor

Erm, can't make out any changes since last time o.O

Contributor

CH4Code commented Mar 16, 2017

Erm, can't make out any changes since last time o.O

@ltem

ltem approved these changes Mar 16, 2017

6PLAY3 fixed

@CH4Code

That last change finally fixed all optical conversion problems. thumbsup

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 17, 2017

Member

Rebased again.

Erm, can't make out any changes since last time o.O

I squashed the changes into existing commits. I just added 80f4fee#diff-54b547985b15dbd63a5da7db1eb43a54R422 (the tile 579 is part of the template 101) and 80f4fee#diff-54b547985b15dbd63a5da7db1eb43a54R413 (tile 342 is part of template 220).

Member

abcdefg30 commented Mar 17, 2017

Rebased again.

Erm, can't make out any changes since last time o.O

I squashed the changes into existing commits. I just added 80f4fee#diff-54b547985b15dbd63a5da7db1eb43a54R422 (the tile 579 is part of the template 101) and 80f4fee#diff-54b547985b15dbd63a5da7db1eb43a54R413 (tile 342 is part of template 220).

@reaperrr reaperrr merged commit d0e859c into OpenRA:bleed Mar 18, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Mar 18, 2017

@abcdefg30 abcdefg30 deleted the abcdefg30:d2kTiles branch Mar 18, 2017

@reaperrr reaperrr modified the milestones: Next Release, Next + 1 Sep 1, 2017

@CH4Code CH4Code referenced this pull request Nov 10, 2017

Open

Finalize the D2k mod #7751

22 of 42 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment