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

Nod Mission 2a #7393

Merged
merged 1 commit into from Feb 6, 2015

Conversation

Projects
None yet
9 participants
@42foobar42

42foobar42 commented Jan 27, 2015

This is the first Nod Mission from this closed PR #7387
I think I implemented all changes according to your comments.

I think I should explain how we worked to create this maps. We gone through all ini files of the original C&C maps and try to implement all triggers with lua. We used this site to understand what every trigger does: https://searchcode.com/codesearch/view/78104/.
I think not everything in this guide is correct.

So all lua function are named after the trigger of the original ini file.

There is still no real AI, but I implement the production trigger according to guide above and I think the behaviour of the units in this map is similar to the original one.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Jan 28, 2015

I forgot somehting to mention.

At the top of the lua file are some functions which are surrounded by the comment "helper functions". Regarding to my expierence in map scripting you need this functions in many Maps and maybe it would make sense to added this function to lua API of openra.

42foobar42 commented Jan 28, 2015

I forgot somehting to mention.

At the top of the lua file are some functions which are surrounded by the comment "helper functions". Regarding to my expierence in map scripting you need this functions in many Maps and maybe it would make sense to added this function to lua API of openra.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Jan 28, 2015

Member

spaces -> tabs please.

Member

abcdefg30 commented Jan 28, 2015

spaces -> tabs please.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Jan 28, 2015

Contributor

Just a couple more notes before I call it a day:

  • There's a problem with the transparency of the map.png. The black background shows up as magenta in-game.
  • I don't know for sure what the tech tree in the 2nd mission was, but I am pretty sure that there weren't guard towers, SAM sites, radar, airfield and flame throwers available. Those should be disabled. I'm not sure about the e3s.
  • I haven't looked at the code in detail yet, but while playing I noticed that the AI builds quite a number of infantry. This should probably be reduced a bit to better fit the difficulty of a second mission.
Contributor

obrakmann commented Jan 28, 2015

Just a couple more notes before I call it a day:

  • There's a problem with the transparency of the map.png. The black background shows up as magenta in-game.
  • I don't know for sure what the tech tree in the 2nd mission was, but I am pretty sure that there weren't guard towers, SAM sites, radar, airfield and flame throwers available. Those should be disabled. I'm not sure about the e3s.
  • I haven't looked at the code in detail yet, but while playing I noticed that the AI builds quite a number of infantry. This should probably be reduced a bit to better fit the difficulty of a second mission.
@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Jan 29, 2015

@obrakmann

  1. I could not reproduce this issues. When and where does it happen?
  2. You were right. There were some of the entries missing. But you also mention the guard tower which is an gdi unit. So the question is, do I have also to write the gdi tech tree in the nod missions.
  3. I try the original mission and there is also a lot of rebulding. They holding the same amount of units till there is no cash (tiberium) left. This is the main difference to the original game. The lua function "produce" does not check if there is any cash left and does not reduce the amount cash for the unit. I did not check if there are some lua function with that you can implement this.

42foobar42 commented Jan 29, 2015

@obrakmann

  1. I could not reproduce this issues. When and where does it happen?
  2. You were right. There were some of the entries missing. But you also mention the guard tower which is an gdi unit. So the question is, do I have also to write the gdi tech tree in the nod missions.
  3. I try the original mission and there is also a lot of rebulding. They holding the same amount of units till there is no cash (tiberium) left. This is the main difference to the original game. The lua function "produce" does not check if there is any cash left and does not reduce the amount cash for the unit. I did not check if there are some lua function with that you can implement this.
@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Jan 29, 2015

Contributor
  1. I could not reproduce this issues. When and where does it happen?

Err... in the mission selection dialog. It's kinda hard to miss :) (unless it's just me)

  1. do I have also to write the gdi tech tree in the nod missions.

Two issues: (1) yes, you have to define the tech tree for both (or all) sides, unless you're using the skirmish defaults (which should rarely happen in missions), and (2) the guard tower is a gdi-only unit in vanilla td, but in openra both sides have it.

I did not check if there are some lua function with that you can implement this.

Yes, there are. Use the Build() function instead of Produce(). See gdi04a/b for usage examples.

You can also check a player's resources with the Cash, ResourceCapacity and Resources properties, but manually managing that is normally not necessary.

Contributor

obrakmann commented Jan 29, 2015

  1. I could not reproduce this issues. When and where does it happen?

Err... in the mission selection dialog. It's kinda hard to miss :) (unless it's just me)

  1. do I have also to write the gdi tech tree in the nod missions.

Two issues: (1) yes, you have to define the tech tree for both (or all) sides, unless you're using the skirmish defaults (which should rarely happen in missions), and (2) the guard tower is a gdi-only unit in vanilla td, but in openra both sides have it.

I did not check if there are some lua function with that you can implement this.

Yes, there are. Use the Build() function instead of Produce(). See gdi04a/b for usage examples.

You can also check a player's resources with the Cash, ResourceCapacity and Resources properties, but manually managing that is normally not necessary.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Jan 29, 2015

I changed point 2 and 3.

The map is shown correct at my laptop.
menu

42foobar42 commented Jan 29, 2015

I changed point 2 and 3.

The map is shown correct at my laptop.
menu

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jan 30, 2015

Member

I also get the pink background on OSX, so this will be a mono vs .NET incompatibility:

screen shot 2015-01-30 at 13 53 16

Make sure that your map.png is encoded using the same properties as our other map previews.

Member

pchote commented Jan 30, 2015

I also get the pink background on OSX, so this will be a mono vs .NET incompatibility:

screen shot 2015-01-30 at 13 53 16

Make sure that your map.png is encoded using the same properties as our other map previews.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Jan 30, 2015

@pchote
I don't know if I fixed it, because I don't use OSX and so I could not test it.
Frankly, I don't know how to change the encoding of an png file nad could not figure it out with google. So I just copyed the map into an old map file. Can you please let me know if it worked out?

42foobar42 commented Jan 30, 2015

@pchote
I don't know if I fixed it, because I don't use OSX and so I could not test it.
Frankly, I don't know how to change the encoding of an png file nad could not figure it out with google. So I just copyed the map into an old map file. Can you please let me know if it worked out?

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Jan 30, 2015

Contributor

Yep, the png is fine now.

Edit: Another thing I forgot to mention is that we use different unit tooltips in missions. You can see how we set those up in map.yaml here, for example.

Contributor

obrakmann commented Jan 30, 2015

Yep, the png is fine now.

Edit: Another thing I forgot to mention is that we use different unit tooltips in missions. You can see how we set those up in map.yaml here, for example.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Jan 31, 2015

Member

Can you squash your commits with interactive rebase?

Member

Mailaender commented Jan 31, 2015

Can you squash your commits with interactive rebase?

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Jan 31, 2015

I hope it was correct what I did...

42foobar42 commented Jan 31, 2015

I hope it was correct what I did...

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Jan 31, 2015

Member

Actually our guide is a bit incomplete. I want you to not only remove the merge commits, but also squash the fixup commits. See https://help.github.com/articles/using-git-rebase/ on how to do that.

Member

Mailaender commented Jan 31, 2015

Actually our guide is a bit incomplete. I want you to not only remove the merge commits, but also squash the fixup commits. See https://help.github.com/articles/using-git-rebase/ on how to do that.

@obrakmann obrakmann referenced this pull request Feb 1, 2015

Open

Add C&C:TD campaign #4988

6 of 8 tasks complete
@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 1, 2015

@Mailaender
Was the last commit correct? Otherwise soembody may guide me through this via irc.

42foobar42 commented Feb 1, 2015

@Mailaender
Was the last commit correct? Otherwise soembody may guide me through this via irc.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 1, 2015

Member

Almost. It looks like you just forgot to update your copy of the bleed branch and rebased onto an old one. Try the following:

git remote add openra https://github.com/OpenRA/OpenRA.git
git fetch openra
git rebase openra/bleed
git push origin bleed --force
Member

Mailaender commented Feb 1, 2015

Almost. It looks like you just forgot to update your copy of the bleed branch and rebased onto an old one. Try the following:

git remote add openra https://github.com/OpenRA/OpenRA.git
git fetch openra
git rebase openra/bleed
git push origin bleed --force
@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 1, 2015

@Mailaender
You were right! I did what you recommended, so think it is fine yet...

42foobar42 commented Feb 1, 2015

@Mailaender
You were right! I did what you recommended, so think it is fine yet...

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 1, 2015

Member

You fixed the libpng transparency error, but added some stretching to the file which is very noticeable when switching missions in the Nod campaign:

output_uigolo

Member

Mailaender commented Feb 1, 2015

You fixed the libpng transparency error, but added some stretching to the file which is very noticeable when switching missions in the Nod campaign:

output_uigolo

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 1, 2015

Member

Sorry for those code change suggestions this spam. :P

I know it's much..., but could you remove all Health: 1 and Facing: 0 values (in map.yaml)?
(See #7319.)

Member

abcdefg30 commented Feb 1, 2015

Sorry for those code change suggestions this spam. :P

I know it's much..., but could you remove all Health: 1 and Facing: 0 values (in map.yaml)?
(See #7319.)

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 1, 2015

@Mailaender
changed size

@abcdefg30
changed everything you mention. I removed also all Facing with another value than 0.

42foobar42 commented Feb 1, 2015

@Mailaender
changed size

@abcdefg30
changed everything you mention. I removed also all Facing with another value than 0.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 1, 2015

Member

I removed also all Facing with another value than 0

Sorry, I think you missunderstood, Facing: 0 is wrong, the other values are ok.

Member

abcdefg30 commented Feb 1, 2015

I removed also all Facing with another value than 0

Sorry, I think you missunderstood, Facing: 0 is wrong, the other values are ok.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 1, 2015

Member

The map image seems be just one pixel off (you can see it when switching between missions).

Member

Mailaender commented Feb 1, 2015

The map image seems be just one pixel off (you can see it when switching between missions).

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 1, 2015

next try ;)

42foobar42 commented Feb 1, 2015

next try ;)

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 1, 2015

Member

👍 I hope the extensive review was also fruitful for you and not just a burden.

Member

Mailaender commented Feb 1, 2015

👍 I hope the extensive review was also fruitful for you and not just a burden.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 1, 2015

Sometimes it was a burden ;) Just kiddin

When I started I thought this would be a lot of reviewing and fixing. So it is fine to do this way and next maps will be faster to rework for me and for you may not such much commenting.

42foobar42 commented Feb 1, 2015

Sometimes it was a burden ;) Just kiddin

When I started I thought this would be a lot of reviewing and fixing. So it is fine to do this way and next maps will be faster to rework for me and for you may not such much commenting.

Show outdated Hide outdated OpenRA.sln
@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 2, 2015

Member

Needs another rebase.

Member

abcdefg30 commented Feb 2, 2015

Needs another rebase.

Show outdated Hide outdated OpenRA.sln
@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 3, 2015

delete changed rows.

42foobar42 commented Feb 3, 2015

delete changed rows.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 3, 2015

Member

I filed 42foobar42#1 in order to get rid of some minor issues.

Member

abcdefg30 commented Feb 3, 2015

I filed 42foobar42#1 in order to get rid of some minor issues.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 3, 2015

Member

Please rebase against bleed and repush to get rid of the merge commit.
You can (should) squash my commit as well.

Member

abcdefg30 commented Feb 3, 2015

Please rebase against bleed and repush to get rid of the merge commit.
You can (should) squash my commit as well.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 4, 2015

How can I retrigger Appveyor? According to the console the error has nothing to do with my changes.

42foobar42 commented Feb 4, 2015

How can I retrigger Appveyor? According to the console the error has nothing to do with my changes.

@Phrohdoh

This comment has been minimized.

Show comment
Hide comment
@Phrohdoh

Phrohdoh Feb 4, 2015

Member

@42foobar42 Someone with access to our AppVeyor builds has to restart it.

Member

Phrohdoh commented Feb 4, 2015

@42foobar42 Someone with access to our AppVeyor builds has to restart it.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Feb 4, 2015

Member

#7408 will fix it.

Member

Mailaender commented Feb 4, 2015

#7408 will fix it.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Feb 5, 2015

Contributor

One last thing we like to do in our mission ports is having the player's starting units move into the map instead of keeping the awkward arrangement the original maps have.

It's a bit of annoying work to do, and has a slight catch, so I submitted a PR on your branch.

Otherwise, I think we're done here. 👍 once that change has been integrated.

Contributor

obrakmann commented Feb 5, 2015

One last thing we like to do in our mission ports is having the player's starting units move into the map instead of keeping the awkward arrangement the original maps have.

It's a bit of annoying work to do, and has a slight catch, so I submitted a PR on your branch.

Otherwise, I think we're done here. 👍 once that change has been integrated.

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 5, 2015

I merged your changes.

Sounds good, if we finally done ;)

42foobar42 commented Feb 5, 2015

I merged your changes.

Sounds good, if we finally done ;)

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 6, 2015

Member

In order to not delay merging this too much,
I suggested some new changes: 42foobar42#3

👍 after that.

Member

abcdefg30 commented Feb 6, 2015

In order to not delay merging this too much,
I suggested some new changes: 42foobar42#3

👍 after that.

fixed mission 2a and cleaned the code
changed according style code and added production trigger

corrected tech tree

removed comments and corrected same style issues

added gdi tech tree and changed produce to build

changed image format and replace spaces with tabs

deleted unnecessary tabs, added tooltips and corrected name

added lua file from nod 2a

changed according style code and added production trigger

deleted unnecessary tabs, added tooltips and corrected name

fixed image size

changed for loops to Utils.Do, removed facing and health from yaml,
changed movement function and corrected mission description

restored facing values unequal zero und fixed image position

Polish things a bit

Have player's units move into the map at the start

Polish some things

obrakmann added a commit that referenced this pull request Feb 6, 2015

@obrakmann obrakmann merged commit aafde15 into OpenRA:bleed Feb 6, 2015

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Feb 6, 2015

Contributor

Changelog

Thanks!

Contributor

obrakmann commented Feb 6, 2015

Changelog

Thanks!

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 6, 2015

Member

Good job. :)

Member

abcdefg30 commented Feb 6, 2015

Good job. :)

@42foobar42

This comment has been minimized.

Show comment
Hide comment
@42foobar42

42foobar42 Feb 6, 2015

Thank you! You're welcome!

So I can start the implementaion of mission 2b on this fork or is there a reason for a new fork?

42foobar42 commented Feb 6, 2015

Thank you! You're welcome!

So I can start the implementaion of mission 2b on this fork or is there a reason for a new fork?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Feb 6, 2015

Member

I don't think you need a new fork, but a new branch may be good.

Member

abcdefg30 commented Feb 6, 2015

I don't think you need a new fork, but a new branch may be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment