Now, that's confusing...

[RaananW]

Hi All,

 

I know this is due to the will to stay backwards-compatible, but this code - https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.mesh.ts#L1424-L1455 is totally not readable.

Lines like this:

if (scene === undefined || !(scene instanceof Scene)) {   if (scene !== undefined) {       sideOrientation = updatable || Mesh.DEFAULTSIDE;       updatable = scene;   }   scene = <Scene>subdivisions;   subdivisions = 1;}

are very problematic. For example:

• scene = <Scene>subdivisions; ? Casing a number to a scene variable, and then setting the subdivisions to a default 1.
• using instanceof in Javascript (for example : !(scene instanceof Scene)) is not the best possible solution. Sometimes instanceof is a must, but JS is not a traditional OO language, instanceof is NOT what you expect it to be.

Is it really a must to stay so backwards-compatible ? JS doesn't have the possibility to overload a function (and the way it is done in TypeScript is a hack, since you need to eventually cast some number variable to a scene variable due to arguments-handling).

There is also a different way of doing that (using the arguments variable? using abstract variable names?), which will be readable (well, a bit more than the current version). The question is - it is so important to leave everything as it was?

 

This has nothing to do with the one who wrote the code (Jerome? DK?) - it had to be implemented this way, this is about the attempt to satisfy everyone :-) I understand why this was done the way it was, I just think it is problematic... Hard to debug, hard to read, hard to develop further. 

[jerome]

I agree 200 %

Its uglissimo.

 

This part of code is a old backward compatibility from DK (adding the optional subdivisions parameter before the required scene parameter), that I over-loaded to ensure a new backward compatibility (adding the {options} parameter).

 

I initially got rid of this old backward compatibility by making  the subdivisions parameter then required and no longer optional. This leaded to the same consistent code as for other CreateXXX() methods with both signatures : list of parameters or {options}.

 

But as you read in the linked topic, the need for old-backward compatibility was expressed. So, it is now necessary to have a double check on the signature and on what is optional or not... so unreadable code and quite un-maintainable imho. I hate it.

 

Fortunately, it's only in CreateCylinder().

 

So only two ways :

• convince everyone that subdivisions should become required in the parameter list signature (good luck)
• find any other elegant hack (in TS !) to recode this in a better readable way (good luck also )

I opted for the first choice because it was the lightest in my opinion and the change was quite minor : not full retro-compat, but really little impact.

 

But I definitely agree with you.

[reddozen]

At some point the engine has to make the decision to break backwords compatibility for certain features if better methods of coding would not allow for it. As you said jerome, the impact of this would be minimal I would think.

[RaananW]

It is not only the cylinder.

 

https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.mesh.ts#L1379 - typeof options should never be number...

https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.mesh.ts#L1398 - instanceof scene...

https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.mesh.ts#L1465 - this is very confusing.

 

I can keep on searching :-)

[jerome]

Yep

On every other CreateXXX(), there is the double signature check but it's more readable than the triple-optional check for the Cylinder.

 

As far as I read about TS signature overloading, there's no way to do differently.

 

I suggested once  :

• to factorize the logic (the code dealing with the mesh geometry), and to call it, say, AbstractCylinderBuilder() for instance
• to provide two different public functions to create the mesh, example : CreateCylinder(listOfParameter) - so nothing changes, no overloading - and BuildCylinder({options}), or any more pertinent name, each function calling the same AbstractBuilder with their own parameters.

but it increases the number of functions (not accepted) ... although the BuildCylinder() function would probably then slowly become the final one to be used because it is does the same than CreateCylinder() but it is extensible, so further need to re-change its signature to extend its features.

[monkeyface]

Personally I don't see the need for Babylon to be that backwards compatible...

Most games/demos coded against the old versions of Babylon will be able to continue to use those for as long as they need. Any new apps can benefit from better performance, features and structure in the newer versions.

The only case where it's useful is where people have ongoing projects that they're still working on or maintaining, but I think almost all of us in that situation are generally willing to do a little fixing up here and there in order to get the benefits of the latest. 

 

I think the importance should be placed on making sure that breaking changes are well documented and the fixes are explained with good quality examples, rather than avoiding from making them at all.

[GameMonetize]

I'm definitely the guy who wants the backward compatibility. This is something really important for people reading blogs, comments or forums about babylon.js. We have NO WAY to guarantee that all the code out there will be updated if we break the compatibility. This will then lead to frustration for any new comers who just want to learn babylon.js. And we ALL know that no one reads the fucking manual.

 

But I'm also really concerned by the complexity induced by the TS overloads and I dislike it.

 

So this is what I suggest: let's revert signatures to old version and add a new meshBuilder.ts which will be a static class providing all new signatures. I worked a lot recently to reduce the file size of babylon.js so we have more room to add files now

 

What do you think ?

[GameMonetize]

Ok let's see where this goes

 

I created the meshBuilder and updated the createBox:

https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.meshBuilder.ts

https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.mesh.ts#L1374

 

 

I think this is FAR better! thank you Raanan for starting this thread and thank you Jerome for the great idea

[jerome]

It seems really clear.. as for the user as for the coder

 

Just wasted so much time to implement these f%*$%king overloaded signatures ! grrrr

[JCPalmer]

How about a 2, 3, or N release rule?  Do whatever it takes to avert breaking in the release that introduced the change, & add a depreciation message including the version # it was changed in & the version it will break in.  It is crucial that users not taking advantage of new features not be forced to make changes, and then not be able go back because of instability later discovered.

 

Question is also, does anyone ever display the browser console?

 

I recently depreciated Mesh.updateVerticesDataDirectly (I am probably the only user), did not say when it will die though.  Saw that an old setVerticesData arg order change message was still there from like 1.3.

[jerome]

 

I recently depreciated Mesh.updateVerticesDataDirectly (I am probably the only user), did not say when it will die though.  Saw that an old setVerticesData arg order change message was still there from like 1.3.

 

Some doc somewhere about how to replace it ? I justly wanted to use it to improve the SPS later

[JCPalmer]

Basically, setVerticesData() & updateVerticesData() can now be passed a number[] or Float32Array.  If it is passed a Float32Array, the throwaway Float32Array is never created for transmission to the GPU.  It is also now saved in the VertexBuffer class in whatever format you passed, in case you did not save the reference.  You can use the same one over and over.  They hold the values from the last update.  This accomplishes what updateVerticesDataDirectly() was for.

 

One trick I also added is to just same Float32Arrays used for CPU skinning, by calling Mesh.setPositionsForCPUSkinning() & Mesh.setNormalsForCPUSkinning().  These only do something the first time, but return the arrays used for CPU skinning.  Just update those, then pass them to updateVerticesData().

 

If you are actually performing CPU skinning, do not do the updateVerticesData() call, if you know for see that bones have also changed that frame.  Let Mesh.applySkeleton() do it for you later in the render process.  This means you can morph in a mesh with a skeleton, where ever bone processing is performed.

 

You really need a high level of control to do both at the same mesh, though.

[jerome]

ok, thank you

[GameMonetize]

I appreciate what you did on updateVerticesDataDirectly. I'm considering keeping this message for 1 major version. For instance the 1.3 warning was removed a few days ago

[RaananW]

I really like the MeshBuilder and the way it is now in the mesh. Now it is backwards compatible and looks really clean :-)

Sorry, never meant to force more work on everyone, I think the mesh class is already so big, it is great to see it smaller :-)

[GameMonetize]

I can't agree more:)

 

More shapes were moved: disc, sphere, lines, ribbon, dashed lines, torus, knot, cylinder

[Dad72]

Just to know, what's the Babylon.core.js file? I tested and I got an error on the plugin physic Oimo.

 

 

[jerome]

@dad72 : http://doc.babylonjs.com/generals/Framework_versions#core-version-babyloncorejs-introduced-in-23

 

@DK, Raanan : So MehsBuilder becomes the place to go, right ?

I guess this is where the upcoming things have then to be.

 

btw, I just had a look here : https://github.com/BabylonJS/Babylon.js/blob/master/src/Mesh/babylon.meshBuilder.ts

it's really clean now 

 

 

I'll need to update the doc (next week), about the CreateXXX() functions with the options parameter also.

Some PG won't work any longer (almost all mines only), but that doesn't really matter. I'll update the examples also.

 

 

Let me know, dear users, when a PG example link does'nt work.

[GameMonetize]

Ok all shapes moved to MeshBuilder!

 

@Jerome: yes this is the place to go now . Thank you for updating the doc and your examples

 

 

One question: should we keep CreatePolyhedron in mesh now? this is a new function introduced in 2.3 and we are still in the alpha version so this kind of things can change. It is an open question actually: should we keep entry point in Mesh class even for new shapes that we can introduce in the future? I would say yes at least for the sake of logic

[RaananW]

If all functions stay in the Mesh class for now, I think the new one should be there as a reference as well. But for the sake of future support, implement it in the MeshBuilder. When it is time to cleanup the mesh class completely, this function can go away as well.

The documentation should direct to the mesh builder.

[jerome]

I suggest that every new (from 2.3) CreateXXX() is implemented directly and only in MeshBuilder... if it has only the {options} signature. This is the case for CreatePolyhedron()

 

The Mesh class could be the legacy class for the CreateXXX(listOfParam) methods and no longer grow.

 

just my opinion, as I didn't intend to implement the future CreateXXX() methods with a list of parameters, but only with an options object parameter

[GameMonetize]

The only problem is more for beginners who will find a discrepancy between mesh and meshbuilder.

 

I agree with Raanan here. Let's just add a proxy in Mesh for now

[jerome]

ok

I didn't read the Mesh class so far, I thought there was still some code creating the meshes inside, but there are only references to MeshBuilder functions

 

So I agree also 

"Creating stuff" code in MeshBuilder and light references (short list of parameters) in Mesh for future CreateXXX() methods. 

[GameMonetize]

We're good to go then gentlemen!

[iiceman]

So this is final, right? Or are you guys still working on this? I think I found a bug, or maybe I am misunderstanding something here. Playground: http://www.babylonjs-playground.com/#2L5NQW

 

I use the MeshBuilder to create parameter versions of boxes. For width (box2) and height (box1) it seems to work well, but for length (box3) it somehow doesn't. My fault or a bug?