I'm implementing a capture animation- QUESTIONS...

Avatar

By presstabstart 23 Jul 2016 07:38

New member · 6 comments

OK so I'm attempting to implement the capture animation I and I have a couple of questions.

1. I've made the capturing mechanism to be more like the one outlined in Bulbapedia, where you have 4 shakes and with each shake there's a chance of the monster escaping the ball. I can always revert this easily, but who do I discuss this change with if I want it in?

2. My progress has been good so far, but I needed to change some things (quite a lot, actually) outside combat_animations.py. These include changes to the code for the item menu states, the item component itself, and the techniques component. Do I need to submit these changes as multiple pull requests, or do I just PR everything at once when I'm finished? This is through multiple commits

To elaborate, I needed to change the item and technique components so the capture() function returns the number of shakes before the tuxemon escapes the ball, and to determine whether or not it succeeded. The return value of item.use is now a named tuple with the name of the last effect used, whether or not it succeeded, and a 'properties' dictionary for misc parameters, such as whether or not to tackle and the number of shakes before the monster escapes the ball. This seemed like a good idea because of the "# TODO: a real check or some params to test if should tackle, etc" comment line.

3. There have been some changes to how targets are selected in combat; now, when in combat, the item target selection menu is the technique target selection menu. Will this be an issue? I don't see any other way to target hostile tuxemon. The item target selection menu is the exact same outside of combat.

4. Will I need to create an issue on Github before I can submit the PR when I'm done? It's on the roadmap, but I'm pretty sure I'll need to create an issue or discuss it with the forum.

thanks, and apologies if these seem like stupid questions. I'm quite new to Github and the practices I need to follow.

oh and the fork's here: https://github.com/presstabstart/Tuxemo … evelopment

Last edited by presstabstart (23 Jul 2016 08:29)

Avatar

By xirsoi 23 Jul 2016 13:41

Champion · 66 comments

I'm not yet qualified to answer most of these questions, but I'll try.

1. bitcraft and ShadowApex are the big men on campus and have contributed the most code. They can answer your questions best.

2. Just one big PR should be fine.

3. I believe bitcraft's changes to target selection included the ability for times/techniques to limit who they can target, which might help you. I suggest reviewing this pull request: https://github.com/Tuxemon/Tuxemon/pull/152/files

4. It would be a good idea to create the issue, if only to have a place in the repo to discuss the change. You can then reference the issue in commits and the pull request name to create a link between the two.

Welcome to the project!

P.S. I thought it would be cool if the number of shakes the tuxemon gets before being captured was a property of the capture device. This would provide another way to improve devices.

Last edited by xirsoi (23 Jul 2016 13:43)


Avatar

By presstabstart 23 Jul 2016 14:00

New member · 6 comments

Thanks xirsoi I'll look through that PR now. Your idea about the number of shakes could be easily implemented. I'll work on getting the animation over and done with before I submit an issue.

Avatar

By tamashihoshi 23 Jul 2016 16:20

Champion · 251 comments
presstabstart wrote

I've made the capturing mechanism to be more like the one outlined in Bulbapedia, where you have 4 shakes and with each shake there's a chance of the monster escaping the ball. I can always revert this easily, but who do I discuss this change with if I want it in?


About shakes:
Should the capturing device shake after catching the tuxemon, and then the tuxemon can break out?
or:
Should the capture device effect spin around the tuxemon, for each "shake" the lines get closer and then go back... if it works, the tuxemon is captured; if not, the lines break. not sure if my description is helpful though.

...should I do some mockup animation? big_smile

oh and welcome to tuxemon~ big_smile


We'll meet again, don't know where, don't know when. But I know we'll meet again some sunny day!

Avatar

By benneti 23 Jul 2016 17:39

Member · 38 comments

I think tamas idea is great, but instead of  breaking in case of a failed catch the lines could just fly away

Avatar

By ShadowApex 24 Jul 2016 03:16

Lead Developer · 374 comments
presstabstart wrote

2. My progress has been good so far, but I needed to change some things (quite a lot, actually) outside combat_animations.py. These include changes to the code for the item menu states, the item component itself, and the techniques component. Do I need to submit these changes as multiple pull requests, or do I just PR everything at once when I'm finished? This is through multiple commits

Small pull requests are actually preferred. A pull request should aim to implement a single feature or bug fix. Bundling too many features and fixes into one takes longer to review and test and can sometimes conflict with other people's changes.


Avatar

By presstabstart 24 Jul 2016 04:21

New member · 6 comments
ShadowApex wrote
presstabstart wrote

2. My progress has been good so far, but I needed to change some things (quite a lot, actually) outside combat_animations.py. These include changes to the code for the item menu states, the item component itself, and the techniques component. Do I need to submit these changes as multiple pull requests, or do I just PR everything at once when I'm finished? This is through multiple commits

Small pull requests are actually preferred. A pull request should aim to implement a single feature or bug fix. Bundling too many features and fixes into one takes longer to review and test and can sometimes conflict with other people's changes.

Do you know how to split a fork into multiple, smaller ones through several commits?

tamashihoshi wrote
presstabstart wrote

I've made the capturing mechanism to be more like the one outlined in Bulbapedia, where you have 4 shakes and with each shake there's a chance of the monster escaping the ball. I can always revert this easily, but who do I discuss this change with if I want it in?


About shakes:
Should the capturing device shake after catching the tuxemon, and then the tuxemon can break out?
or:
Should the capture device effect spin around the tuxemon, for each "shake" the lines get closer and then go back... if it works, the tuxemon is captured; if not, the lines break. not sure if my description is helpful though.

...should I do some mockup animation? big_smile

oh and welcome to tuxemon~ big_smile

I was thinking of the 1st option. If you're saying you want to program the animation yourself, then I can submit the issue + PR right now and you can work on what I already have. I really don't mind!

Last edited by presstabstart (24 Jul 2016 04:24)

Avatar

By ShadowApex 24 Jul 2016 08:46

Lead Developer · 374 comments
presstabstart wrote

Do you know how to split a fork into multiple, smaller ones through several commits?

The best way to do this is to create a separate branch in your fork for each feature you work on. Then when you open a pull request, you can select the source branch with your changes.


Avatar

By presstabstart 24 Jul 2016 09:29

New member · 6 comments
ShadowApex wrote
presstabstart wrote

Do you know how to split a fork into multiple, smaller ones through several commits?

The best way to do this is to create a separate branch in your fork for each feature you work on. Then when you open a pull request, you can select the source branch with your changes.

Honestly, and this was definitely stupid of me, but I laid out more than 1 change in a commit and so splitting the fork doesn't seem possible. If you really need it as multiple forks I could just reproduce everything again from another fork but that could take a while.

Last edited by presstabstart (24 Jul 2016 09:30)

Avatar

By ShadowApex 24 Jul 2016 10:07

Lead Developer · 374 comments
presstabstart wrote

Honestly, and this was definitely stupid of me, but I laid out more than 1 change in a commit and so splitting the fork doesn't seem possible. If you really need it as multiple forks I could just reproduce everything again from another fork but that could take a while.

Don't worry about it. It may just take a little longer to review and test. Smaller pull requests are preferred but aren't mandatory. smile


Avatar

By presstabstart 24 Jul 2016 10:18

New member · 6 comments
ShadowApex wrote
presstabstart wrote

Honestly, and this was definitely stupid of me, but I laid out more than 1 change in a commit and so splitting the fork doesn't seem possible. If you really need it as multiple forks I could just reproduce everything again from another fork but that could take a while.

Don't worry about it. It may just take a little longer to review and test. Smaller pull requests are preferred but aren't mandatory. smile

ok thanks.

I split my fork into 3 different branches for 3 different PRs and 3 different issues anyway, since it implements 3 (mostly small) features.
If I did anything wrong apologies again. I might have to update some of the PRs though if you choose to not accept the latest. but no worries, it's not a big deal  smile

Last edited by presstabstart (24 Jul 2016 10:48)

Avatar

By bitcraft 25 Jul 2016 02:17

Champion · 166 comments

Hey presstabstart, I have done most of the combat animations and the combat state.  If you'd like to chat on IRC (my preference!), I'm on most of the time.  I leave IRC open 24/7, and I'd encourage you to as well, as the channel is slow, but there are occasionally busts of activity that you may find useful to read later, even if you are not at your computer.

That said, I've looked over your changes, and I'll give you my comments here.

  • Changing the item use menu was fine.  In the future, we may add the option to select other monsters in the party, but not in play.  The change is needed to use the capture device, so its all good.
  • Target selection is a work in progress.  A week or so ago, the changes for the 1st part were merged...than enabled the selection box for techniques.  While there is targeting data in the json, I haven't gotten to making use of the defaults (so potions go to your team, and capture device select your opponent.)  I've started the 2nd part that selects the correct targets and enforces so rules (so you cannot use the capture device on your monsters), but I haven't opened a PR yet.  There is some discussion here: https://github.com/Tuxemon/Tuxemon/issues/129.
  • It seems that capturing doesn't add/remove monsters.  Personally, I'd like to see that implemented using events...so do a "remove_monster" event for the opponent, then a "add_monster" for the player.  You may need to implement a check and a dialog that says "Gotcha!" and what not.  You can save these changes for another PR
  • For the result, you seem to be mixing the use of attributes and the "properties" dictionary...it just seems cleaner to me if you implemented it completely as a namedtuple or a dictionary.
  • If you are going to add a new translation key ("attempting_capture"), please also update en_US.json

While I'm not an expert git user, I'll just outline my work flow... The 3 PR's you opened took a while for me to figure it all out, and in the future, you may consider how I work to limit the burden of checking PRs (I'm not an expert, but I think this method is easier for maintainers)

  • Open an issue about your ideas first...  I tend to be pretty verbose, but I almost always open an issue before I even start coding.  I think that your changes are fine, overall, but opening an issue first and making a quick discussion about how to plan to make the changes will make the PR checking step much faster, and eliminate any wasted effort if your changes are not accepted.
  • When you begin to code, from a clean fork (git reset --hard upstream/master), make a feature branch for whatever you are doing
  • If you've finished a feature that you plan another PR for, then make a new branch.
  • Before you open a PR, squash merge for 1st feature branch to your development branch to keep a clean history
  • Only open a PR from your development branch to the upstream development branch, unless it is a simple change
  • If you are working on other features that have dependencies to your first PR, only squahs-merge your first branch to development and wait for your PR to be merged...then squash-merge the second branch and open a new PR

Again, that just how I do it, and there are many, many ways (because git lets you)...

Finally, thanks for your hard work!  I'm super excited that you took one of the more challenging parts of the game (combat and animations) and made some great changes.  I think it needs a bit more polish, and I'm sure you'll get comments from other people, but overall, this is great.  It will be awesome to having a working capture device and see the first "Gotcha!".

Avatar

By presstabstart 25 Jul 2016 07:23

New member · 6 comments

Thanks Bitcraft. I wasn't completely confident that I'd be able to complete the combat animation at first so I didn't really create an issue or thread before I was sure I could tackle it. I'll create issues from now on.

I'll implement the locale change right now. Unfortunately I don't know any other languages, so I can only update en_US.

About the data structure of the return value of .use() , I don't think a named tuple would be appropriate since people would have to add to it each time they added an effect. A dictionary seems nicer. I'll change it now. Same with the locale.

Oh and cheers for the guide on submitting PRs. I wasn't really sure how this whole thing worked (as you can see). My whole PR situation is a mess right now though, should I remove all but the latest PR? I've already made some changes to the branch for the latest PR that would be useful for the others but changing all 3 would take time and I could mess up somewhere. Or should I just change the earliest PR, then remove the other 2 PRs and wait for the earliest one to get accepted?

thanks for the work you've done on the combat+animations. The whole phases and animation system is really convenient and really easy to work with. Very intuitive  smile

For now I'm going to delete the unwanted PRs and refer all issues to the latest PR. The animation itself right now is a prototype open for much discussion, but it works.

Avatar

By bitcraft 25 Jul 2016 15:31

Champion · 166 comments

Updating just en_US is totally fine.  We have some tools to check which translations are missing and treat the en_US locale as "master", and all other translations must follow it.

I can't take total credit for combat.  I just used the nice plan that was laid out by ShadowApex and implemented it.  I envisioned it to use a more modular system, "Phase" classes and a true FSM, but it seems a bit more simple as-is.

As for the animations, I'd like to see more of them worked into the game.  It can be used to fade things in and out, smoothly move things around, etc.  More little animations like that will make the game look/feel much better.

https://github.com/Tuxemon/Tuxemon/wiki … e-Overview

IMO, the PR you have up is ok size-wise, but you could have split it into "implement capture" and "change capture alg".  I wouldn't split it now though, just my thoughts...

Everything is looking pretty good so far!