How to contribute without breaking something?

[adam]

This is a question to the people who contribute code to BabylonJS.  How do you do it without breaking some other code you are not familiar with?  Are there any tests that you run before committing?

[NasimiAsl]

hi adam .

 

what you means ? about code all 3d engine is independed code.

 

you can make it with php and asp and mvc and solid html .

 and dont need change any code you have it because all engine begin in client side and result here  and all source inside BAYBYLON plugin and you just create a instance and start your scene .

 

i dont understand your question thanks @RaananW 

[RaananW]

That's a good question.

 

Running the gulp typescript build is a start. TypeScript is type-safe, so technically you would find out if you made silly JS mistakes (naming a variable wrong etc').

Other than that - use the compiled code to run a simple scene - see that it is working well. Also create a demo scene for the feature you have just changed / created and see that it works. That's the best way of testing.

I was working on Mocha-Testing (was hoping to be finished before 2.3 is out. I still hope). But even with selenium integrated, those tests cannot fully check a 3D scene. this is a task for two eyes and a mouse.

 

Make sure you understand how git works. If you don't, simply ask! It is hard at the beginning and easy afterwards!

 

Make sure you only commit .ts files. .js are automatically compiled.

 

Write clean code, try to stay conform to the BJS coding style. It is rather standard. Keep your code formatted.

 

Don't break functionality. Everything should be backwards-compatible. This is important for previously created playgrounds, and a simple upgrade for a newer version.

 

Have fun!

 

BTW - even if you do make a mistake, we will let you know after reviewing your commit. We will then ask you to repair it, and will be more than happy to show you how!

[adam]

As an example, CreateLathe is still not working correctly.  I got it to work on my end by modifying ExtrudeShapeGeneric.  Other code depends on ExtrudeShapeGeneric.  If there was a test I could run to make sure that I didn't break any functionality, I would feel more comfortable contributing my code.

[Vousk-prod.]

An other advice I'd like to add to Raanan's: versionning is about coding bricks, when using git, if you commit only once a day you're probably not using it efficiently, a commit should contain only the changes actually related to the unit thing you're currently modifiying.

Two differents points equal two differents commits, two differents functionnalities equal two differents pull requests.

This will help you and others to easily check code and track potential conflicts.

Sounds obvious, but better be said than ignored.

[RaananW]

As an example, CreateLathe is still not working correctly.  I got it to work on my end by modifying ExtrudeShapeGeneric.  Other code depends on ExtrudeShapeGeneric.  If there was a test I could run to make sure that I didn't break any functionality, I would feel more comfortable contributing my code.

Well, that's an issue that should be addressed! why isn't it working corrctly? most of our bug reports are coming from the users themselves!

 

About testing - as I said, hopefully 2.3 final will contain tests already. If not 2.3 then the next version. That I promise

[adam]

Well, that's an issue that should be addressed! why isn't it working corrctly? most of our bug reports are coming from the users themselves!

 

About testing - as I said, hopefully 2.3 final will contain tests already. If not 2.3 then the next version. That I promise

 

http://www.html5gamedevs.com/topic/14939-new-mesh-type-lathe/?p=91689

 

The way it looks with my modifications to ExtrudeShapeGeneric:

http://www.babylonjs-playground.com/#1KN1LB#32

 

Current state:

http://www.babylonjs-playground.com/#1KN1LB#35

[Vousk-prod.]

@Raanan: Adding tests is nice but we should be aware that it does not make the size of the BJS lib increase ! We are on the web, size matters.

[RaananW]

tests are always external, don't worry.

[G'kar]

Hi, reading this forum thread remind me that I wrote a few notes on the step I followed to my first contribution to babylonJS core.

 

Here it is for you (provided as is, initially for my own reminder, may be worth sharing ? I let you see).

Nothing new in fact, just take me some time to get through the process, collecting pointer to information, so would be happy if this can help other to find such info more quickly than me. (as i wrote some time ago, I was also wondering how to test my change versus any regression tests )

 

Workflow for babylonjs contribution

1) Typescript vs Javascript
 babylon used javascript, then moved to typescript language for coding.
Ideas behind the choice are available here : http://blogs.msdn.com/b/eternalcoding/archive/2014/04/28/why-we-decided-to-move-from-plain-javascript-to-typescript-for-babylon-js.aspx?WT.mc_id=16566-DEV-codeproject-article51
So now source file are a mix of .ts and .js files.
.ts is the reference, but still possible to summit change as .js (up to project leader to merge the changes proposal to .ts)
I quote from ... : "... to educate developers to try to have less contributions in JavaScript and more in TypeScript."
Look like there is many advantages to use typescript, recommended !
so now, what to do it for the coding work-flow ? see below


2) file generation summary

x.ts -> x.js  \
y.ts -> y.js  |
..ts -> ..js  |-> babylon.js, babylonmax.js, ...
..ts -> ..js  |
z.ts -> z.js  /

tools are :
- typescript compiler (.ts -> .js)
- dependencies analysis : gulp+node.js

3) build commands:
prepare: npm gulp install
refresh all .js from .ts : gulp typescript
idem+including declaration file: gulp typescript-compile
have auto refresh in background when you save your file : gulp watch-typescript

4) tools install
4.1) tools install Linux
Good starting point at : "Build Babylon.js with Gulp"
  https://github.com/BabylonJS/Babylon.js/tree/master/Tools/Gulp
Gulp is claimed as cross platform:

npm (= nodejs package manager)
intalling npm to get gulp ...
  seem to get trouble with nodejs : "wanted: {"node":">=0.12.0"} (current: {"node":"v0.10.25","npm":"1.3.10"})"
  I install v4.2.1 as told in https://nodejs.org/en/download/package-manager/
  In fact false issue (just wrong idea to put Babylon.js on samba net drive, links are not working well with gulp on CIFS disk)
  Waste lot of time on this stupid one.

 

npm and gulp is all you need. The rest of the dependencies are defined in the package.json file, located in the Gulp directory. Including typescript.

So, all you need to do is 

> Install NodeJS

> Install Gulp:

  npm install -g gulp

> Install the dependencies

  npm install

 

Optional:
Get typescript on command line : 'sudo npm install -g typescript'
Then you can compile typescript with 'tsc your_source_file.ts'

4.2) tools install Windows
TBD (not using windows at the moment, can check later)
typescript supported in visual studio

5) prepare a contribution
Just have to edit the .ts file to add your feature
Regenerate the babylon.js file from the source .ts
Test this is as you expect (using the above build command)

Question: Is there any basic regression test procedure available, or any list of test to do to see if basic things are broken ?
Or do you throw your change and rely on other to detect your bug for you ?
(Sorry for the though, I am sure no one can leave any bug in his perfect code)

6) Pull request process
repository is github hosted.
- Create a github account if you don't have one already (easy on https://github.com/)
- Fork th babylon.js project (see how on https://help.github.com/articles/fork-a-repo/)
- Clone the just fork repo to your local machine
  'git clone https://github.com/YOUR-USERNAME/Babylon.js.git YOUR-HOME-DIRECTORY-TO-WORK-ON'
- create your branch with your contribution
  To be defined
- issue a PR=pull request for your contribution to be merged to main branch of development
- Admin/proj leader/... will considerer your change (good/bad/to be adjusted/discussed ...)
- hopefully you get your contrib integrated in the main development flow (in the upcoming release )
  YOUR-USERNAME/Babylon.js --> BabylonJS/Babylon.js

Edited December 20, 2015 by G'kar

[jerome]

@adam : sorry, for the late answer, I thought a PR with your fix was already done.

 

Actually, the bug is only about manually capped lathes as I remember.

Adam found a fix to this dedicated case and I wanted, at this moment, to take the time to check if the lathe geometry could be improved more generally to solve this.

 

And I forgot ... 

The SPS implementation and my conf took my mind away from this.

 

 

 

So I think that I will, in early january, recode the Adam's fix in TS and PR it. This will help people using the lathe and wanting to cap it manually.

 

Then I'll have time  :

 

1 ) to try to implement the ribbon closeArray smooth seam as the closePath seam, what is quite complex to do.

The tube relies on the closed ribbon with closePath and is now really wroking in terms of uvs and normals, even when morphed. The lathe relies on the closed ribbon with closeArray . Maybe improving the closePath will solve all of the sudden the lathe manual cap.

 

2 ) else to check accurately how to improve the lathe geometry so it could take generally in account the cases of manual capping without using any dedicated process.

[jerome]

@G'Kar

 

Very interesting report on BJS contribution workflow !

[RaananW]

Hi G'Kar,

 

Nive review :-)

here are my notes:

 

tools are :

- typescript compiler (.ts -> .js)
- dependencies analysis : gulp+node.js+???

npm and gulp is all you need. The rest of the dependencies are defined in the package.json file, located in the Gulp directory. Including typescript.

So, all you need to do is 

1) Install NodeJS

2) Install Gulp:

npm install -g gulp

3) Install the dependencies

npm install

4) Enjoy.

 

You can of course (as you stated later) install typescript globally, but it is not needed for the project. using gulp typescript (or the watch variant) will keep your js files compiled in the correct way with the correct dependencies.

 

Question: Is there any basic regression test procedure available, or any list of test to do to see if basic things are broken ?

Or do you throw your change and rely on other to detect your bug for you ?
(Sorry for the though, I am sure no one can leave any bug in his perfect code)

TypeScript's major feature is type safety (one thing that lacks in javascript :-) ). If you try to compile babylon after making a change, this will be the first thing you need to check. If your code compiles, you used the right types in the right areas (except for when you "cheat" ad use "any" as type constantly. In that case you lose the typescript benefit).

 

The Repo's admins will look at your code, compile a scene or two with it (if it is needed) and approve or deny (usually - will tell you what's wrong).

 

As I said - unit tests are on the making. Our ToDo list is sadly (or happily ) very large, and this is the only holdback.

 

 

6) Pull request process

repository is github hosted.
- Create a github account if you don't have one already (easy on https://github.com/)
- Fork th babylon.js project (see how on https://help.github.com/articles/fork-a-repo/)
- Clone the just fork repo to your local machine
  'git clone https://github.com/YOUR-USERNAME/Babylon.js.git YOUR-HOME-DIRECTORY-TO-WORK-ON'
- create your branch with your contribution
  To be defined
- issue a PR=pull request for your contribution to be merged to main branch of development
- Admin/proj leader/... will considerer your change (good/bad/to be adjusted/discussed ...)
- hopefully you get your contrib integrated in the main development flow (in the upcoming release )
  YOUR-USERNAME/Babylon.js --> BabylonJS/Babylon.js

That's correct. there are a few other steps, but this is already git related. The most important one is merging Babylon's master to your branch before making a pull request, to make sure no conflicts occurred.

If any one needs git help, give us a shout - we will be more than happy to explain. We will help anyone who will approach nicely and ask a question. Was like that so far, and will stay like that in the future

[G'kar]

Thanks for feedback.

I try to edit my post to reflect some of your comments (trying to avoid confusion for other).

[jerome]

@adam :

 

Looking at your code : http://www.babylonjs-playground.com/#1KN1LB#32

At the line 142, you clone the model shape as it is for the first iteration of the lathe extrusion.

 

This works well for the lathe because the model shape is defined in the plane xOy. So the clone (first iteration) is also in the same plane and then we extrude by rotating around the Y axis.

 

But ExtrudeShapeGeneric must work for any kind of extrusion, not only rotation around the Y axis.

I'm afraid this code fixes only the Lathe but breaks every other extruded shapes.

 

Nevertheless, reading your code leaded me to something particular : the Lathe is the only pre-build extruded geometry needing to "close" its underlying ribbon path array.

And the ribbon parameter closeArray still needs the same (complex) optimization than the one done for the parameter closePath. So I need to dig in this way.

 

Unless it's in the Lathe geometry itself ...

 

[EDIT] : I'm checking, I think it's in the lathe geometry itself ... I'll let you know

[jerome]

Ok  got it :

 

Not that simple to explain, but an extrusion is built on top a Path3D and as a Path3D can't know how its path will evolve after its start, the first position of an extruded shape is always orthogonal to the path (in the plane normal/binormal actually). It's not aware of any kind of external rotation center.

 

But we expect it to be radial when we design a circular lathe.

 

In brief, ExtrudeCustomShape() won't fit for the lathe. I will recode a ribbon-based dedicated geometry for this.

[jerome]

new lathe geometry PR

 

I tested it : no more artifact at all

[jerome]

I just added to the Lathe object the feature cap.

Same values and behavior than for the tube : http://doc.babylonjs.com/tutorials/Mesh_CreateXXX_Methods_With_Options_Parameter#tube

 

It adds automatically some cap(s) where you ask for.

var lathe = BABYLON.MeshBuilder.CreateLathe("l",{shape: shape, cap: CAP_NUMBER}, scene);

[jerome]

@adam : the existing PG with the updated geometry => http://www.babylonjs-playground.com/#1KN1LB#35

 

I simplified then the model shape and here is the same with the cap parameter : http://www.babylonjs-playground.com/#1KN1LB#36

Don't care about the little error on the bottom cap, it's already fixed in github, it will automatically disappear when the ficed will be pushed in the PG engine.