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

Create new EdgesRenderer for LineMeshes. #4919

Merged
merged 5 commits into from Aug 10, 2018
Merged

Conversation

barteq100
Copy link
Contributor

@barteq100 barteq100 commented Aug 10, 2018

Normal EdgesRenderer was buggy because of unnecessary triangulation of lines.

Easy way to see difference is to run this code on playground:

var createScene = function () {

    // This creates a basic Babylon Scene object (non-mesh)
    var scene = new BABYLON.Scene(engine);

    // This creates and positions a free camera (non-mesh)
    var camera = new BABYLON.FreeCamera("camera1", new BABYLON.Vector3(0, 5, -10), scene);

    // This targets the camera to scene origin
    camera.setTarget(BABYLON.Vector3.Zero());

    // This attaches the camera to the canvas
    camera.attachControl(canvas, true);

    // This creates a light, aiming 0,1,0 - to the sky (non-mesh)
    var light = new BABYLON.HemisphericLight("light1", new BABYLON.Vector3(0, 1, 0), scene);

    // Default intensity is 1. Let's dim the light a small amount
    light.intensity = 0.7;

     var line = new BABYLON.MeshBuilder.CreateLines('line', {points: [
        new BABYLON.Vector3(1,0,7),
        new BABYLON.Vector3(2, 0 ,7),
        new BABYLON.Vector3(4,0,3)
    ]}, scene);

    line.enableEdgesRendering();

    return scene;

};
```

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

Hello, and thanks for the pull request.

It seems like a few javadoc documentation are missing on the new API:
[10:53:44] Missing text for parameter generateEdgesLines (id: 32765) of constructor (id: 32760) babylon.d.ts:28757
[10:53:44] Missing text for Class : LineEdgesRenderer (id: 32812) babylon.d.ts:28895

@barteq100
Copy link
Contributor Author

should I just make javaDoc comments in .ts files?

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

yup but I am sorry I said Javadoc... :-( , my bad.

We are using Typedoc to generate our api doc so that our documentation should follow the following standards:

http://doc.babylonjs.com/how_to/contribute_to_api

Do you mind adapting it ?

A second question is the work you are adding on line mesh is great. I am still wondering wether it is an issue with the default edge renderer and what in line mesh is creating the issue ?

@barteq100
Copy link
Contributor Author

barteq100 commented Aug 10, 2018

Sure I will adapt.

And yes it was a problem with a standard edges renderer. It basicaly tried to go through 3 vertices for a line while there were only 2 so it iterated past the array getting Vector Zero as a last vertex per triangle.

@barteq100
Copy link
Contributor Author

barteq100 commented Aug 10, 2018

When you will have some time please look at the docs I made and tell me what should I change.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, we do not need type annotations and @Class as this is infered from typescript by typedoc :-)

Aside of comments it looks all good.

@@ -2,7 +2,7 @@
export class LinesMesh extends Mesh {
public color = new Color3(1, 1, 1);
public alpha = 1;

public _edgesRenderer: Nullable<LineEdgesRenderer>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add
/**

to prevent public undocumented APIs


/** Gets or sets a boolean indicating if the edgesRenderer is active */
public isEnabled = true;

// Beware when you use this class with complex objects as the adjacencies computation can be really long
constructor(source: AbstractMesh, epsilon = 0.95, checkVerticesInsteadOfIndices = false) {
/**
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a quick desc

constructor(source: AbstractMesh, epsilon = 0.95, checkVerticesInsteadOfIndices = false) {
/**
*
* @param {BABYLON.AbstractMesh} source Mesh used to create edges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the {Types}

/**
*
* @param {BABYLON.AbstractMesh} source Mesh used to create edges
* @param {number} epsilon sum of angles in adjacency to check for edge
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

looks good, I ll try to merge it in the afternoon !

Thanks a lot.

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

Oh no, just noticed, could you remove the dist/preview release/babylon.d.ts from the PR ?

I promise it is the last comment I have, thx

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

Also the EdgesRenderer is missing a constructor Comment which fails the build.

I ll merge as soon as the d.ts is removed from the PR and the comment has been put in the constructor.

@sebavan sebavan mentioned this pull request Aug 10, 2018
@sebavan sebavan merged commit bf566f5 into BabylonJS:master Aug 10, 2018
@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

I took the liberty of fixing the left over issues and merged on master in #4920

Thanks for your contribution !

@barteq100
Copy link
Contributor Author

Thank you for merging and fixing issues. I had to urgently leave so I didn't see your last request

@sebavan
Copy link
Member

sebavan commented Aug 10, 2018

np

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

Successfully merging this pull request may close these issues.

None yet

2 participants