Gaussian Splatting Utilities #1
Conversation
Sorting and texture generation utilities for gaussian splats
|
Hi @keyboardspecialist, is this PR ready for suggestions on project infrastructure and npm module configuration? |
There was a problem hiding this comment.
Thanks @keyboardspecialist! I'm excited to see that we're taking advantage of WASM to optimize CesiumJS runtime performance. Hopefully this can serve as a model for future optimizations!
I reviewed this PR through the lens of:
- project setup for a Cesium open source repository
- general best practices for testing, documentation, and CI
- setup for publishing an npm package
I did not review any of the rust code or build processes and would recommend a second reviewer for that aspect.
| @@ -0,0 +1,84 @@ | |||
| <div align="center"> | |||
There was a problem hiding this comment.
I assume we'll want to customize this README to be specific to this project, correct?
When this is published on npm, this (or another README in a distribution directory) will serve as the landing page and should include setup instructions and usage examples. For instance, see @cesium/engine, a package in the CesiumJS monorepo, or the Draco npm package for how a wasm package is positioned.
There was a problem hiding this comment.
I would also suggest adding the following files at the repository root:
README.md: This can be fairly minimal with a brief description of project motivation and a link to this subfolder.CONTRIBUTING.md: You can borrow the relevant parts of CesiumJS's. Definitely include a link to the Cesium Code of Conduct and instruction for completing the CLA
| @@ -0,0 +1,25 @@ | |||
| Copyright (c) 2018 Jason Sobotka <jason.sobotka@cesium.com> | |||
There was a problem hiding this comment.
Cesium projects typically only use Apache 2.0 licenses (I believe this is due to better patent protection rights).
| @@ -0,0 +1,201 @@ | |||
| Apache License | |||
There was a problem hiding this comment.
Is there reason for this to be different than the root project license? If not, I would remove it in favor of the root license.
| @@ -0,0 +1,69 @@ | |||
| language: rust | |||
There was a problem hiding this comment.
We have moved our repos to GitHub actions rather than Travis for CI.
Can we do the same here?
| ### 🎁 Publish to NPM with `wasm-pack publish` | ||
|
|
||
| ``` | ||
| wasm-pack publish |
There was a problem hiding this comment.
As we are planning on publishing this to npm, I would expect to see a package.json file committed somewhere to the repo.
It looks like wasm-pack will generate this file for us as a start. The scope to use here would be cesium. However, my guess is that we'll need to make some modifications to the auto-generated version to conform to our other npm packages.
There was a problem hiding this comment.
We'll also want to think through what the release process for this package entails. We should document it, similar to (but I'm sure not nearly as extensive as) the CesiumJS Release Guide, and automate any particularly manual portions if it makes sense.
| } | ||
|
|
||
| //Algorithm from ILM | ||
| //https://github.com/mitsuba-renderer/openexr/blob/master/IlmBase/Half/half.cpp |
There was a problem hiding this comment.
This is licensed under BSD 3-clause, which is all good. 👍
Just a heads up, if you're not already aware– We need confirm that code is licensed in a compatible way with Apache 2.0 before we can reference it for our own projects.
| @@ -0,0 +1,11 @@ | |||
| install: | |||
There was a problem hiding this comment.
Was this intentionally included, or was this an autogenerated file?
We're looking to standardize on GitHub Actions for CI wherever possible.
| @@ -0,0 +1,31 @@ | |||
| [package] | |||
| name = "wasm-splats" | |||
| version = "0.1.0" | |||
There was a problem hiding this comment.
I assume this should be 1.0 by the time we merge this PR. Correct?
| [package] | ||
| name = "wasm-splats" | ||
| version = "0.1.0" | ||
| authors = ["Jason Sobotka <jason.sobotka@cesium.com>"] |
There was a problem hiding this comment.
I think this should specify "Cesium GS, Inc."
| # logging them with `console.error`. This is great for development, but requires | ||
| # all the `std::fmt` and `std::panicking` infrastructure, so isn't great for | ||
| # code size when deploying. | ||
| console_error_panic_hook = { version = "0.1.7", optional = true } |
There was a problem hiding this comment.
As mentioned in my previous comment, I don't see this being used anywhere. Based on this comment, it should probably be removed.
Summary
WebAssembly package for CesiumJS for working with Gaussian Splats. Currently has sorting and attribute texture generation.