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

Mercs now start with weapons in their inventory #110

Merged
merged 3 commits into from
Aug 27, 2016

Conversation

JGelfand
Copy link
Contributor

The planets.json config file has the mercenary item listings, and neither one was starting with weapons in their inventory. I added weapons, and everything else worked ok. I also added a placeholder block for code for mercenaries equipping items dropped on them. I don't think we should have them do that, though- not having to worry about equipping mercenaries is easier for the player.

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@JGelfand
Copy link
Contributor Author

Ah yes- I should probably add that this resolves issue #109, and also that I really should have payed attention when reading #101, where @Cervator clearly stated that

NPC ships were no longer equipping stuff

(emphasis mine).

Anyway, the next time a experienced member of the project mentions something, I'll try to pay attention XD. At least I only wasted four hours... T_T.

@Cervator
Copy link
Member

Hehe sorry about the hassle! But thank you so much for figuring this one out :-)

@JGelfand
Copy link
Contributor Author

Do you want to have me work on making mercenaries equip items dropped on them, or do you just want to quickly remove the placeholder and pull?

@Cervator
Copy link
Member

Please leave the placeholder in place, it'll be good to have it handy. I'll merge this soon'ish today, catching up after having mostly missed a day or two :-)

Equipping items dropped on the mercenaries can be done as a separate issue any time later. Maybe we should add a few more options too, I never actually noticed there were only two of them enabled.

@Cervator
Copy link
Member

Tested on both PC and Android, looking good! Thanks again :-)

@Cervator Cervator added the bug label Aug 27, 2016
@Cervator Cervator added this to the v1.4.1 milestone Aug 27, 2016
@Cervator Cervator merged commit fc22815 into MovingBlocks:develop Aug 27, 2016
@Cervator
Copy link
Member

Oh, a Git protip as well: Whenever upstream (MovingBlocks) changes favor updating in these two ways:

  • If you have no changes locally use git pull to simply add the new commits on top of your local workspace - no merge commit
  • If you do have changes locally use git rebase to sort of sneaky merge in the upstream changes to actually appear to have happened before your changes (it moves your commits till the "head" of the branch) - no merge commit

Both of those approaches keep the history graph cleaner as it avoids merges in favor of just moving commits around cleanly :-)

So right now with this merged assuming you have nothing outstanding locally you should be able to simply pull latest from upstream (git pull upstream develop would be one way that would work via command line, if upstream is the name you gave a git remote link to MovingBlocks)

@JGelfand
Copy link
Contributor Author

Thanks for the tip. I'm currently even with MovingBlocks, but next time I get behind I'll try this, I guess.

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

Successfully merging this pull request may close these issues.

3 participants