search

MrNeRF / gaussian-splatting-cuda

3D Gaussian Splatting, reimagined: Unleashing unmatched speed with C++ and CUDA from the ground up!
Other
909 stars 74 forks source link

Lower CUDA requirement to 11.7, auto-detect CUDA_ARCHITECTURES #23

Closed paulmelis closed 1 year ago

paulmelis commented 1 year ago

I can't judge any performance implications, but the lower compute capability makes it possible to run on A30, A40, A100 and RTX 3050-3090 (amongst others). The current compute_89 is very restrictive ;-)

MrNeRF commented 1 year ago

Yeah I know. This was also on my TODO list. Going down is either not the best option :) You will loose the optimizations for your arch provided by the nvcc compiler. Do you have time to look into this? https://stackoverflow.com/questions/68223398/how-can-i-get-cmake-to-automatically-detect-the-value-for-cuda-architectures There is a solution to automatically derive the architecture. Super easy and straight forward. That also implies that there will be a lower limit, probably below 80. But as long nobody is complaining, we should not catch this.

Going with a more recent cmake version (2.24) appears most elegant to me? What do you think? This would be my favorite.

Btw, please look at the contribution guidelines in the README.md. That mostly means squashing commit for the pr. Otherwise it is really hard to review it properly. Thanks a lot.

MrNeRF commented 1 year ago

Btw, the Readme needs then also an update with respect to the requirements.

paulmelis commented 1 year ago

Going with a more recent cmake version (2.24) appears most elegant to me? What do you think? This would be my favorite.

Cmake 3.24 (assume you mean that) was released around a year ago, seems fair to set that as the minimum version. And if that sets CUDA_ARCHITECTURES automatically that should indeed be an easy way. I'll see if I can make the changes.

Btw, the Readme needs then also an update with respect to the requirements.

Yes indeed, I noticed that only later

Btw, please look at the contribution guidelines in the README.md. That mostly means squashing commit for the pr. Otherwise it is really hard to review it properly. Thanks a lot.

Yeah, I noticed that comment. I'm not entirely clear what to do different in my git workflow to actually squash commits I'm afraid. Is it merely using --squash when doing a pull?

MrNeRF commented 1 year ago

I also just learning the github workflow. At work we have bitbucket and plastic scm. That becomes annoying when you have multiple commits and you have conflicts on master. Also the reviewing sucks then. Anyway, maybe I should become this things first straight for myself. https://chat.openai.com/share/944c67d3-555e-4524-b2f0-81481f9cd5ba So maybe lets stick with what we have for now and I just relax the squashing requirement. Most importantly, all commits to master have to compile. Then you can use bisect to find an issue in a very systematic way. But this should the reviewer also help to make sure.

paulmelis commented 1 year ago

I think you might need to enable something on the repo, if I read https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests correctly?

paulmelis commented 1 year ago

Using cmake 3.24 and auto-detect of CUDA arch works fine here, nice.

MrNeRF commented 1 year ago

looks good to me. You can add it also to the news update on the top of the Readme. Btw, we have also tested with a 3090 Ti. That works. The 3080Ti has issues with larger datasets as reported in the issue section. That will be hopefully also tackled today. You might make these updates? I will test it on my computer and then merge it given that it works on my computer as well.