search

Kitware / Danesfield

Kitware's system for 3D building reconstruction for the IARPA CORE3D program
Apache License 2.0
155 stars 56 forks source link

3dtiles pointcloud and mesh processing #40

Closed danlipsa closed 1 year ago

danlipsa commented 2 years ago

@jacobderosa Please review.

danlipsa commented 1 year ago

@dmjoy @jacobderosa Please review. Lets merge this!

danlipsa commented 1 year ago

Note the number of commits is misleading as I moved git@gitlab.kitware.com:core3d/danesfield-conda-recipes.git to the conda-recipes directory including history.

jacobderosa commented 1 year ago

@danlipsa I have small changes to Dockerfile, tools/run_danesfield.py, and the VTK recipe that I need to merge. Otherwise this looks good

dmjoy commented 1 year ago

@danlipsa overall looks fine to me, I would recommend changing out how the SSH stuff is done in the Dockerfile though. Also, regarding the number of commits, if you did something like a subtree add, you can usually add a --squash flag to have it combine all commits into a single one. Probably not worth re-doing it in this case, but I would recommend squashing this PR on merge (so that we don't drown out the history of the main project).

danlipsa commented 1 year ago

@dmjoy I agree, we should squash. I don't think the recipe's history is very important.

danlipsa commented 1 year ago

@jacobderosa Are you going to add your changes here?

danlipsa commented 1 year ago

Or maybe pickup this branch and create a different PR?

danlipsa commented 1 year ago

@jacobderosa I changed danesfield-conda-recipes repo -> conda-recipes dir to have only one commit. Diff seems to show nothing changed. Please review and merge.

danlipsa commented 1 year ago

Not sure if you addressed the suggestion from @dmjoy regarding SSH of if you not doing that at all now. That seems that something we should do.

jacobderosa commented 1 year ago

@danlipsa I'll add that along with my own changes after this is all merged. For now this looks good to me

danlipsa commented 1 year ago

I have a few more commits I added when I worked on when added offset inside GLTF. I'll rebase and open another pull request.