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

Convert Mesh to TypeScript #6382

Merged
merged 4 commits into from
Feb 4, 2020
Merged

Convert Mesh to TypeScript #6382

merged 4 commits into from
Feb 4, 2020

Conversation

Zyie
Copy link
Member

@Zyie Zyie commented Feb 3, 2020

Description of change

Converts Mesh to TypeScript. Nice and simple

Pre-Merge Checklist
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@Zyie Zyie changed the title Dev mesh typescript Convert Mesh to TypeScript Feb 3, 2020
@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Feb 3, 2020
Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev left a comment

Choose a reason for hiding this comment

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

Good job! Oh wait, its draft...

@@ -399,7 +427,7 @@ export class Mesh extends Container
}
else
{
this.uvs = geomUvs.data;
this.uvs = geomUvs.data as Float32Array;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use ITypedArray in such places in future, but its fine to convert it if we know certain type.

@@ -477,20 +505,21 @@ export class Mesh extends Container
this.geometry.dispose();
}

this.geometry = null;
(this as any).geometry = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@eXponenta changed geometry to private field + readonly prop, but for destroy both ways are fine and I want to underline that by approving this PR :)

/**
* @param {Float32Array|number[]} vertices - Positional data on geometry.
* @param {Float32Array|number[]} uvs - Texture UVs.
* @param {Uint16Array|number[]} index - IndexBuffer
*/
constructor(vertices, uvs, index)
constructor(vertices: IArrayBuffer, uvs: IArrayBuffer, index: IArrayBuffer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

yay! That's why I created IArrayBuffer.

@Zyie Zyie marked this pull request as ready for review February 3, 2020 20:54
@Zyie
Copy link
Member Author

Zyie commented Feb 3, 2020

@ivanpopelyshev I do have a question for this pr.

Inside MeshGeometry there is _updateId. I couldn't find any reference to it so is it needed?

@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

Merging #6382 into dev will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #6382     +/-   ##
=========================================
- Coverage   74.84%   74.64%   -0.2%     
=========================================
  Files          81       78      -3     
  Lines        4401     4355     -46     
=========================================
- Hits         3294     3251     -43     
+ Misses       1107     1104      -3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0885bca...b065c64. Read the comment docs.

@bigtimebuddy bigtimebuddy merged commit d4c7047 into dev Feb 4, 2020
@bigtimebuddy bigtimebuddy deleted the dev-mesh-typescript branch February 4, 2020 00:13
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

4 participants