0ADMods / millenniumad

Millennium A.D. is a mod for 0 A.D. covering the AD 500–1000 timeframe.
http://wildfiregames.com/forum/index.php?showforum=297
GNU General Public License v2.0
35 stars 18 forks source link

Fixes for umayyads-actors #55

Closed mrlie58 closed 4 years ago

mrlie58 commented 4 years ago
Nescio0 commented 4 years ago

The purpose of the opening post is to inform people what the pull request does (and why it's an improvement). You don't have to list every individual file, you can group and summarize your changes, e.g.:

Add map umayyads.xml with all umayyads-actors

git mv umayyads.xml sandbox_umayyads.xml

Merge https://github.com/0ADMods/millenniumad/pull/54 into local repository

That will introduce a merge conflict.

mrlie58 commented 4 years ago

Some of the units previously used objectcolor, which is a perfectly valid material, why did you change it for player color?

I've researched the Main game and found <material>objectcolor.xml</material> not in unit-actors, but in "props" and some "trader_h". Therefore i only changed the norse and anglo actors (forgot anglo_spearman_a and anglo_spearman_b) and leave the others as is.

StanleySweet commented 4 years ago

It can be used anywhere instead of using player color it uses the specified color

StanleySweet commented 4 years ago

Please revert the objectcolor changes.

Nescio0 commented 4 years ago

And please undo your changes to the (six) templates, we don't want a merge conflict, nor regression.

Also rename the map you introduced:

git mv umayyads.xml sandbox_umayyads.xml

mrlie58 commented 4 years ago

Please revert the objectcolor changes.

DONE

Nescio0 commented 4 years ago

DONE

Still not done: 55

mrlie58 commented 4 years ago

DONE

Still not done: 55

I've really problems to correct this ;-) Any of my Editors seems to format the files i have to change. Only the good, old MS-Editor shows me the correct(wrong) indentation. So i will try to just copy an original file from an ealier downloaded milleniumad-mod over the actual file. But i'm afraid to give you more work as necessary. Is there a simplier way to fix this?

Nescio0 commented 4 years ago

@mrlie58, you can do it manually via your webbrowser:

Because you do have to do the files one by one, this is really inefficient for larger pull requests. However, in this case it was just a few lines in two files. As you can see I've already fixed the two offending templates, because it was more work explaining than doing it myself. This patch thus no longer has a merge conflict, but you still have to address @StanleySweet's point, hence why I'm writing this explanation for you nonetheless.

[EDIT] And edit your opening post (i.e. the first message in this conversation) to reflect everything it does, since it includes all your commits.

mrlie58 commented 4 years ago

@mrlie58, you can do it manually via your webbrowser:

* signing in to github

* go to https://github.com/mrlie58/millenniumad

* browse to find the file(s) you want to change

* click on “Edit this file” (the pencil symbol)

* make the changes (or in this case, copy-and-paste the old lines to restore them)

* then click “Commit the changes directly to the master branch” and “Commit changes”.

Because you do have to do the files one by one, this is really inefficient for larger pull requests. However, in this case it was just a few lines in two files. As you can see I've already fixed the two offending templates, because it was more work explaining than doing it myself. This patch thus no longer has a merge conflict, but you still have to address @StanleySweet's point, hence why I'm writing this explanation for you nonetheless.

[EDIT] And edit your opening post (i.e. the first message in this conversation) to reflect everything it does, since it includes all your commits.

Related to Stan's point i've done this at the same time as yours, but i think i've made it wrong because i titled it "adress requested changes (stan)".