DartBox2D patch submit process?

[Rupert Key]

Hi,

What's this project's preferred patch submitting process?  (or project joining process?)

I've got several bugfixes, a minor feature enhancement and some working rope code to submit.

• bug, patch, developer integrates?
• fork, push, pull-request?
• Become member, push changes? :-D
• ...?

Thanks, Rupert.

[dominic]

Hi Rupert

I was just looking into this today as you'd mentioned the other day that you might work on the rope stuff. I'd be very happy to see your contributions.

The first step is having you sign a CLA, which you can sign electronically: http://code.google.com/legal/individual-cla-v1.0.html. Please note, this doesn't take away your ownership of the work you submit, it just allows us the right to use the work.

Once you've signed that, I'll add you as a contributor. From there, it's a matter of submitting a patch for review. It seems that the googlecode code review system relies on the code being submitted before it can be reviewed which is, well, not ideal. I'll look into alternatives, but you could attach a patch to an email to this list just for review before pushing it to the repo.

- dominic

[dominic]

Ok, I've set up a rietveld repository. The base for an uploaded patch should be http://dartbox2d.googlecode.com/git/.

Instructions are now at http://code.google.com/p/dartbox2d/wiki/Contributing.

[Rupert Key]

Awesome!  I'll look at this shortly/later -- currently in the middle of debugging a performance tweak while being shocked at SGS2 (4.0.3) running 11 FPS vs. SGS3 (4.0.4) hitting consistent 60-66 FPS !!!

Can we just upgrade the World to super-fast bleeding edge smartphones? ;-)

[Rupert Key]

Hi, Dominic

I'm back to getting my changes together for a commit (especially since I fear a recent dart2js change has introduced a bug that DartBox2D exposes).

I'm still polishing my rope code but since DartBox2D needs updating for recent language changes anyway, the first commit I'll offer is fixes for those, general bug fixes and a couple of tiny enhancements.  Can I offer a patch of all these together (vs. separate ones for each)?  Here's the proposed commit message.  (I'm still tweaking the patch but hopefully ready to investigate Rietveld tomorrow.)

Updates for Dart language changes + bugfixes...

Updated for string concatenation removal.

Color3f: Added == implementation.

IViewportTransform: Added missing scale setter.

CanvasDraw, DebugDraw, World:

 Fixed broken drawSegment() (its previous mutation of its arguments broke joints when they were rendered).

 Implemented drawTransform().

 Made drawSolidCircle()'s unused "axis" argument optional and API-consistant with drawCircle().

 Added DebugDraw.e_lineDrawingBit and use for selecting line-drawn debug drawing (sometimes clearer for diagnosis).

Minor bug fixes.

Added extra demo "BoxTest" solely demonstrating a bouncing box.

Normally I'd have committed the updates, bug fixes and enhancements as separate batches but the "patch and review" process feels to prefer larger chunks.  Let me know if you'd prefer things differently.

Also, I've submitted my CLA.  LMK if any problems.

Thanks, Rupert.

On Wednesday, June 6, 2012 4:47:28 PM UTC+1, Dominic Hamon wrote:

Ok, I've set up a rietveld repository. The base for an uploaded patch should be http://dartbox2d.googlecode.com/git/.

Instructions are now at http://code.google.com/p/dartbox2d/wiki/Contributing.

[Dominic Hamon]

Hi Rupert

Welcome back!

Thank you for taking on the language changes. It's been on my mind for a while but I have been trying to find the right time to start, knowing that more are coming.

I prefer focused patches, but I think we can strike a balance with yours. How about patches for each of these:

- the new demo

- drawing fixes (CanvasDraw, DebugDraw, World)

- other language fixes and bug fixes

With rietveld it should be possible to create the patches in different git branches, upload the CLs to rietveld, and then after the review you can push the patch.

Have you received confirmation regarding the CLA? I haven't seen the notification yet.

[Rupert Key]

Hi, Dominic

Ha!  Well, I was coming back to this thread to say I've just published the code review at http://codereview.appspot.com/6374063/

...but then I saw your last response here and your comment on dart-discuss re. numerical odditie -- which, as mentioned above, was a large part of my reason for finishing pushing since I hadn't been able to tie it down otherwise!  The new BoxTest.dart is a minimal box2d demo that demonstrates the problem.  (I've updated 4075 with what I know.)

If you'd like me to disentangle the changes, I can probably address tomorrow.  I prefer focused patches too ... but I'll admit I'm not massively keen on redoing things ;-)

If you decide to take a look. the only one you'll likely have issue with is CanvasDraw.  After making so many tweaks to it, I let my "developer" head take over from my "sustaining" head and reordered the methods to a more logical order.  (I'd normally have committed the reorder separately since the changes are quite small otherwise.)

LMK.

Also for discussion, I included a comment in the review re. the generated files (JS and Doc).  Rietveld's upload.py failed to upload all these when I first tried and they really bloat the review so I've left them out for now.  I'd assume to include them in the final push, fair?  (It did occur that customized doc might be preferred?  The default generation includes pages for core Dart classes since they're consumed -- a bit redundant.  It also includes some [admittedly small] PNG binaries -- often not something to committed to a DVCS.)

Rupert.

[Dominic Hamon]

I'll take a look now.

I can review it as one chunk, and then maybe you can carve it up for pushes?

I hit the size limit issue myself trying some test patches. It's a pain but only seems to impact the .js and .map files. I wouldn't normally include them in the repo as they're generated but they are useful for people to run the demo directly from googlecode outside of the VM.

[Rupert Key]

(inline)

On Thu, Jul 12, 2012 at 9:09 PM, Dominic Hamon <dom...@google.com> wrote:

I'll take a look now.

Awesome!  Thanks :-)

 

I can review it as one chunk, and then maybe you can carve it up for pushes?

OK.

So to be clear, are we talking 1 push each for...

- the new demo

- drawing fixes (CanvasDraw, DebugDraw, World)

- other language fixes and bug fixes

... and another for the generated bits?

(vs. one for top 3 and another for generated)

I hit the size limit issue myself trying some test patches. It's a pain but only seems to impact the .js and .map files. I wouldn't normally include them in the repo as they're generated but they are useful for people to run the demo directly from googlecode outside of the VM.

Oh!  I had assumed these were only issues with Rietveld -- not GoogleCode's git, no?

R.

[dominic]

On Thu, Jul 12, 2012 at 1:19 PM, Rupert Key <ruper...@gmail.com> wrote:

(inline)

On Thu, Jul 12, 2012 at 9:09 PM, Dominic Hamon <dom...@google.com> wrote:

I'll take a look now.

Awesome!  Thanks :-)

You should have the first batch of comments now.

 

 

I can review it as one chunk, and then maybe you can carve it up for pushes?

OK.

So to be clear, are we talking 1 push each for...

- the new demo

- drawing fixes (CanvasDraw, DebugDraw, World)

- other language fixes and bug fixes

... and another for the generated bits?

(vs. one for top 3 and another for generated)

You should be able to push the generated bits each time. I'm trying to think of a workaround for having them not in the repo, but the link to the demos has been published.

 

I hit the size limit issue myself trying some test patches. It's a pain but only seems to impact the .js and .map files. I wouldn't normally include them in the repo as they're generated but they are useful for people to run the demo directly from googlecode outside of the VM.

Oh!  I had assumed these were only issues with Rietveld -- not GoogleCode's git, no?

Yes, it's only an issue with Rietveld. It pains me every time I push generated code though.

 

R.

[Rupert Key]

On Thu, Jul 12, 2012 at 9:22 PM, dominic <domi...@google.com> wrote:

On Thu, Jul 12, 2012 at 9:09 PM, Dominic Hamon <dom...@google.com> wrote:

I'll take a look now.

Awesome!  Thanks :-)

You should have the first batch of comments now.

Got 'em.  ... and got your responses to mine!  Wow, this is faster than some companies I've worked at!  Will likely action conclusions tomorrow (getting late here).

 

I can review it as one chunk, and then maybe you can carve it up for pushes?

OK.

So to be clear, are we talking 1 push each for...

- the new demo

- drawing fixes (CanvasDraw, DebugDraw, World)

- other language fixes and bug fixes

... and another for the generated bits?

(vs. one for top 3 and another for generated)

You should be able to push the generated bits each time. I'm trying to think of a workaround for having them not in the repo, but the link to the demos has been published.

OK, sorry to be anal but I want to ensure I get this right so I'm going to Explicit Mode :-)

Once the code review is complete, I will locally commit on main branch (right?) and push the following to GoogleCode git repo (nothing more to Rietveld for these changes):

(N.b. This order assumes the demo might depend on the changes.  1 & 2 may also need swapping depending on interdependencies.)

Push 1:

- language fixes and bug fixes + generated DartDoc output and demo JS files

Push 2:

- drawing fixes (CanvasDraw, DebugDraw, World) + generated DartDoc output and demo JS files

Push 3:

- the new demo (Dart and generated JS file)

Should the commit messages be the (separated) text from the code review body (likely) or these lines above (the bit after the hyphen)?

Do feel free to please correct me in nice explicit and patronizing fashion :-)

Thanks, R.

[dominic]

On Thu, Jul 12, 2012 at 2:06 PM, Rupert Key <ruper...@gmail.com> wrote:

On Thu, Jul 12, 2012 at 9:22 PM, dominic <domi...@google.com> wrote:

On Thu, Jul 12, 2012 at 9:09 PM, Dominic Hamon <dom...@google.com> wrote:

I'll take a look now.

Awesome!  Thanks :-)

You should have the first batch of comments now.

Got 'em.  ... and got your responses to mine!  Wow, this is faster than some companies I've worked at!  Will likely action conclusions tomorrow (getting late here).

You caught me on a 'dart day' :)

 

 

I can review it as one chunk, and then maybe you can carve it up for pushes?

OK.

So to be clear, are we talking 1 push each for...

- the new demo

- drawing fixes (CanvasDraw, DebugDraw, World)

- other language fixes and bug fixes

... and another for the generated bits?

(vs. one for top 3 and another for generated)

You should be able to push the generated bits each time. I'm trying to think of a workaround for having them not in the repo, but the link to the demos has been published.

OK, sorry to be anal but I want to ensure I get this right so I'm going to Explicit Mode :-)

Oo-er.

 

Once the code review is complete, I will locally commit on main branch (right?) and push the following to GoogleCode git repo (nothing more to Rietveld for these changes):

(N.b. This order assumes the demo might depend on the changes.  1 & 2 may also need swapping depending on interdependencies.)

Push 1:

- language fixes and bug fixes + generated DartDoc output and demo JS files

Push 2:

- drawing fixes (CanvasDraw, DebugDraw, World) + generated DartDoc output and demo JS files

Push 3:

- the new demo (Dart and generated JS file)

Should the commit messages be the (separated) text from the code review body (likely) or these lines above (the bit after the hyphen)?

Do feel free to please correct me in nice explicit and patronizing fashion :-)

That all sounds good to me. The commit messages can be whatever you feel necessary to explain your changes. How's that for vague and fuzzy?

 

Thanks, R.