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

fix(tile-converter): featureIds + uvRegions #2588

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

belom88
Copy link
Collaborator

@belom88 belom88 commented Aug 17, 2023

No description provided.

@belom88 belom88 requested a review from ibgreen August 17, 2023 09:19
@@ -212,6 +212,10 @@ function unifyObjectsByFeatureId(
existedObject.texCoords,
currentObject.texCoords
);
existedObject.uvRegions = concatenateTypedArrays(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Existed sounds strange, it is not a noun (it is past tense of a verb) . Not quite sure what it is trying to say but I would recommend that you make another effort to name this variable.

Suggested change
existedObject.uvRegions = concatenateTypedArrays(
existingObject.uvRegions = concatenateTypedArrays(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a loop could be considered here?

for (const attributeName in ['positions', 'normals', ..., 'uvRegions']) {
  existing object[attributeName] = concatenateTypedArrays(...

@belom88 belom88 merged commit ff2a63d into master Aug 21, 2023
1 check passed
@belom88 belom88 deleted the vb/fix-tile-converter-feature-ids-uv-regions branch August 21, 2023 15:02
@dsavinov-actionengine dsavinov-actionengine added the ActionEngine The issue is on ActionEngine controll label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ActionEngine The issue is on ActionEngine controll
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants