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

GreasedLine changes #14943

Merged
merged 25 commits into from Apr 9, 2024
Merged

GreasedLine changes #14943

merged 25 commits into from Apr 9, 2024

Conversation

RolandCsibrei
Copy link
Contributor

@RolandCsibrei RolandCsibrei commented Apr 3, 2024

Added AuTO_DIRECTIONS_FACE_TO mode.
Added support for custom UVs.
Fixed simple material shader for non camera facing mode.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 3, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@Popov72
Copy link
Contributor

Popov72 commented Apr 3, 2024

You should remove the package-lock.json file from your PR.

@RolandCsibrei
Copy link
Contributor Author

You should remove the package-lock.json file from your PR.

Sorry, I didn't even notice it. git add . always worked for me until now...
Why is it not included in .gitignore?

@Popov72
Copy link
Contributor

Popov72 commented Apr 4, 2024

Why is it not included in .gitignore?

I think it's because we still commit this file on our side when necessary, but it's not supposed to be done by the end user. @RaananW will know better than me, though.

Note that you should not remove the file from the repo, but from your PR only :)

Also, you shouldn't have any change in your local package-lock.json file: have you synchronized your local repo with the main repo?

@RolandCsibrei
Copy link
Contributor Author

RolandCsibrei commented Apr 4, 2024

Note that you should not remove the file from the repo, but from your PR only :)

I removed it using the GitHub app on my mobile while I was in the commits section for this PR. I did this for the first time but who in the world could have guessed it will remove it from the repo not from the commits. Sorry :) Should I take any action? What should I do? Thanks!

Also, you shouldn't have any change in your local package-lock.json file: have you synchronized your local repo with the main repo?

Yes, all was up to date. This is why I always use git add . to add all changed files and this is why I was not aware of package-lock.json being added.

@Popov72
Copy link
Contributor

Popov72 commented Apr 4, 2024

You should do a git pull upstream master to get the package-lock.json file back, and commit it in your PR. This should fix it.

@RolandCsibrei
Copy link
Contributor Author

RolandCsibrei commented Apr 4, 2024

@deltakosh there is a commit 182fbcb by you which introduced an error:

packages/dev/core/src/Meshes/GreasedLine/greasedLineBaseMesh.ts:246:9 - error TS2322: Type 'FloatArray' is not assignable to type 'number[]'.
  Type 'Float32Array' is missing the following properties from type 'number[]': pop, push, concat, shift, and 5 more.

246         return this._uvs;

Doesn't count this as a breaking change though? Changing the return type of a public getter? :) I can fix the thing, just don't know what approah you prefer, reverting the commit or I will fix the issue in this PR. Thanks!

We could actually support FloatArray as well:

 // added FloatArray
interface GreasedLineMeshOptions {
    uvs?: number[] | FloatArray;
}
set uvs(uvs: number[] | FloatArray) {
  this._uvs = uvs instanceof Float32Array ? uvs : new Float32Array(uvs);
  this._createVertexBuffers();
 }

@RolandCsibrei
Copy link
Contributor Author

RolandCsibrei commented Apr 4, 2024

Aaaarqgh :-D

[tsl] ERROR in /home/vsts/work/1/s/packages/dev/core/src/Meshes/GreasedLine/greasedLineMesh.ts(180,34)
      TS2802: Type 'Float32Array | number[]' can only be iterated through when using the '--downlevelIteration' flag or with a '--target' of 'es2015' or higher.

Edit:
Just realized that there is a FloatArray alias used in the code... I'm going to adapt the rest to this.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 4, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@RolandCsibrei
Copy link
Contributor Author

RolandCsibrei commented Apr 4, 2024

@Popov72 I've managed to fix the package-lock.json issue. git pull upstream master didn't help, git always reported all up to date. I had to git add ./package-lock.json -f, commit it, push it and then delete the file on my local copy, git add ., commit, push and seems it worked :-)

@RolandCsibrei RolandCsibrei marked this pull request as draft April 8, 2024 02:49
@RolandCsibrei RolandCsibrei marked this pull request as ready for review April 8, 2024 10:58
RaananW
RaananW previously requested changes Apr 8, 2024
Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

package-lock is being deleted (unless I see a wrong state?)

A git reset, reverting this change and force-push a commit to your branch will probably be the best fix here.

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 8, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 8, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@RolandCsibrei
Copy link
Contributor Author

@RaananW is it now ok to merge?

@bjsplat
Copy link
Collaborator

bjsplat commented Apr 9, 2024

Visualization tests for WebGPU (Experimental)
Important - these might fail sporadically. This is an optional test.

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14943/merge/testResults/webgpuplaywright/index.html

@sebavan sebavan merged commit 4d68141 into BabylonJS:master Apr 9, 2024
11 checks passed
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

5 participants