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 examples #162

Closed
wants to merge 12 commits into from
Closed

Conversation

trigpolynom
Copy link
Contributor

@trigpolynom trigpolynom commented Mar 15, 2025

built successfully based on changes made on top of the work @jorge-ortega has already done in #160. Below are some config params I had to set for my Ubuntu 22.04 env:

Set CUDA Root and Paths

export CUDA_ROOT=/usr/lib/cuda
export CUDA_PATH=$CUDA_ROOT
export CUDA_NVVM_ROOT=$CUDA_ROOT/nvvm

CUDA Binary and Library Paths

export PATH=$CUDA_ROOT/bin:$CUDA_ROOT/nvvm/lib64:/root/.cargo/bin:$PATH
export LD_LIBRARY_PATH=$CUDA_ROOT/lib64:$CUDA_ROOT/nvvm/lib64:$CUDA_ROOT/nvvm/libdevice:$LD_LIBRARY_PATH
export LIBRARY_PATH=$CUDA_ROOT/lib64:$CUDA_ROOT/nvvm/libdevice:$LIBRARY_PATH

Rust and OptiX Configuration

export LLVM_LINK_STATIC=1
export RUST_LOG=info
export OPTIX_ROOT=$HOME/optix/
export OPTIX_ROOT_DIR=$OPTIX_ROOT

Add Rust Toolchain Path

export PATH=$HOME/.cargo/bin:$PATH
export LD_LIBRARY_PATH=$HOME/.rustup/toolchains/nightly-2025-03-02-x86_64-unknown-linux-gnu/lib:$LD_LIBRARY_PATH
export RUSTFLAGS="-C link-args=-ltinfo"

Copy link
Contributor

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the PR! 👏

Some small changes and questions. You can also rebase on main as I landed the PR this depends on.

@@ -55,6 +55,10 @@ pub fn find_cuda_root() -> Option<PathBuf> {
None
}

// pub fn find_cuda_root() -> Option<PathBuf> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out? We use this in other places I believe.

@@ -55,9 +62,9 @@ pub fn run(camera: &Camera, scene: &Scene) -> ! {
let event_loop = EventLoop::new();
let wb = WindowBuilder::new()
.with_title("Render")
.with_inner_size(PhysicalSize::new(WIDTH, HEIGHT));
.with_inner_size(PhysicalSize::new(WIDTH as f64, HEIGHT as f64)); // Explicitly specify f64
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.with_inner_size(PhysicalSize::new(WIDTH as f64, HEIGHT as f64)); // Explicitly specify f64
.with_inner_size(PhysicalSize::new(WIDTH as f64, HEIGHT as f64));

@@ -0,0 +1,45 @@
thread 'opt 4qj5v7pmzdsa87mgb0rgitfl0' panicked at /rustc/8c392966a013fd8a09e6b78b3c8d6e442bc278e1/compiler/rustc_codegen_ssa/src/back/write.rs:916:21:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove.

window::WindowBuilder,
ContextBuilder,
};
// use glutin::{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove commented code.

@LegNeato
Copy link
Contributor

Also be sure to run cargo rustfmt --all and cargo clippy!

@trigpolynom
Copy link
Contributor Author

Thank you for these! I got it to build right before hopping on a flight and forgot to clean some stuff up. Will clean up ASAP and handle formatting

@@ -21,7 +21,6 @@ use optix_device::{

extern "C" {
#[cfg(target_os = "cuda")]
#[cfg_attr(target_os = "cuda", nvvm_internal(addrspace(4)))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this to nvvm_interanal::addrspace(4) on the cuda target.

Copy link
Collaborator

@jorge-ortega jorge-ortega Mar 18, 2025

Choose a reason for hiding this comment

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

We might need to add back L23 to keep this gated only for cuda.

@trigpolynom
Copy link
Contributor Author

trigpolynom commented Mar 16, 2025

Ahhh im afraid its failing on cargo clippy. I spoke prematurely! All comments should be addressed though and I ran rustfmt. @LegNeato and @jorge-ortega . Still succeeding on cargo build though. Will address issues found by clippy.

Copy link
Contributor

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

CI is failing.

@trigpolynom
Copy link
Contributor Author

CI is failing.

Yeah windows build fails and Linux clippy task fails but build succeeds. Will move this in progress. Sorry! Got too excited haha.

@trigpolynom
Copy link
Contributor Author

have clippy running successfully now and rebased on main @jorge-ortega

Copy link
Contributor

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Awesome, getting close! Looks like formatting is still off so CI is failing.

@@ -212,8 +212,8 @@ pub type OptixLogCallback = ::std::option::Option<
pub const OptixDeviceContextValidationMode_OPTIX_DEVICE_CONTEXT_VALIDATION_MODE_OFF:
OptixDeviceContextValidationMode = 0;
pub const OptixDeviceContextValidationMode_OPTIX_DEVICE_CONTEXT_VALIDATION_MODE_ALL:
OptixDeviceContextValidationMode = -1;
pub type OptixDeviceContextValidationMode = ::std::os::raw::c_int;
OptixDeviceContextValidationMode = 4294967295;
Copy link
Contributor

Choose a reason for hiding this comment

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

Still haven't explained why this is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm this seems to be something that changed on build. looking into

Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file is generated through bindgen, and its values are going to differ when building on linux vs windows. The build seems to suggest this file was only written there for debugging, as it also writes it to the output directory, and that one is ultimately included in sys.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it automatically converts -1 to 4294967295 on my machine on compilation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

tl;dr is it should be safe to ignore this file in your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we add to gitignore?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove it and update build.rs so it only writes to the output dir. We can do that in another PR and ignore the file for now.

@@ -155,14 +155,20 @@ pub fn codegen_bitcode_modules(
/// linked when building the nvvm program.
pub fn find_libdevice() -> Option<Vec<u8>> {
if let Some(base_path) = find_cuda_root() {
let libdevice_file = fs::read_dir(Path::new(&base_path).join("nvvm").join("libdevice"))
let libdevice_dir = Path::new(&base_path).join("nvvm").join("libdevice");
println!("Checking libdevice directory: {:?}", libdevice_dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just printing feels wrong here. Can we use the context (I think there is a dcx() somewhere) or make this a trace!() or a debug!() ? Pretty sure rustc has tracing, but I don't know which version it was added in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for debugging and I forgot to remove. My bad! Should be fixed now.

@@ -1139,13 +1134,13 @@ impl From<CurveType> for sys::OptixPrimitiveType {
#[repr(u32)]
#[derive(Copy, Clone, PartialEq)]
pub enum VertexFormat {
None = sys::OptixVertexFormat_OPTIX_VERTEX_FORMAT_NONE as u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing the cast will break on windows. The enum type in the generated bindings are c_int on windows (thus the u32 cast) but c_uint on linux. See rust-lang/rust-bindgen#1361

If clippy is complaining about this, we'll need to ignore them here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was a clippy change. I'll make these changes tomorrow.

@trigpolynom
Copy link
Contributor Author

trigpolynom commented Mar 18, 2025

im getting rustfmt and cargo build to pass using the .github/rust.yml commands but clippy is failing on issues like the following:

help: if you import `c_int`, refer to it directly
    |
204 -             .and_then(|millis| c::c_int::try_from(millis).ok())
204 +             .and_then(|millis| c_int::try_from(millis).ok())
    |

error[E0433]: failed to resolve: use of undeclared type `IFlags`
   --> /home/nonpl/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustix-1.0.2/src/fs/ioctl.rs:149:35
    |
149 |         ioctl::ioctl(fd, ctl).map(IFlags::from_bits_retain)
    |                                   ^^^^^^ use of undeclared type `IFlags`

How is it failing on imported code? @LegNeato and @jorge-ortega, please excuse any non-rust lingo or ignorance on my behalf.

@jorge-ortega
Copy link
Collaborator

Not sure your error is coming from clippy, but rather trying to compile something before clippy runs. Try running cargo tree -i rustix to see what it pulling in the failing crate. My guess it's going to be sysinfo. On windows, I'm not able to compile ntapi, which gets pulled in through sysinfo 0.24.7. We might need a newer version then this to get it to compile on windows.

@jorge-ortega
Copy link
Collaborator

CI passes on windows because we don't build the path_tracer example atm, but the changes don't compile for me on windows.

@trigpolynom
Copy link
Contributor Author

Not sure your error is coming from clippy, but rather trying to compile something before clippy runs. Try running cargo tree -i rustix to see what it pulling in the failing crate. My guess it's going to be sysinfo. On windows, I'm not able to compile ntapi, which gets pulled in through sysinfo 0.24.7. We might need a newer version then this to get it to compile on windows.

hmm that makes sense but cargo build completes successfully for me not just when I run CI checks.

@trigpolynom

This comment was marked as resolved.

@LegNeato
Copy link
Contributor

LegNeato commented Mar 19, 2025

@trigpolynom I'd prefer to not use discord at this point and stick to GitHub (I turned on discussions). I'd prefer the server was shut down or not advertised as associated with the project.

Discord is not nearly as searchable. Over and over again I've seen it drag maintainers down with the same questions. Information and questions are better in GitHub so it is searchable and can be referenced from tasks and issues. I've also seen discord encourage drive by questions. It's easier to just ask than learn, search, read docs, read code, or solve their own issues. Answers almost never make it back to the docs. For whatever reason answers from GitHub discussions more often than not make it back into code and docs in my experience...maybe people are in a different mindset in the GitHub UI 🤷‍♂️.

@trigpolynom
Copy link
Contributor Author

@trigpolynom I'd prefer to not use discord at this point and stick to GitHub (I turned on discussions). I'd prefer the server was shut down or not advertised as associated with the project.

Discord is not nearly as searchable. Over and over again I've seen it drag maintainers down with the same questions. Information and questions are better in GitHub so it is searchable and can be referenced from tasks and issues. I've also seen discord encourage drive by questions. It's easier to just ask than learn, search, read docs, read code, or solve their own issues. Answers almost never make it back to the docs. For whatever reason answers from GitHub discussions more often than not make it back into code and docs in my experience...maybe people are in a different mindset in the GitHub UI 🤷‍♂️.

Understood! Makes complete sense!

@jorge-ortega
Copy link
Collaborator

Don't think this merged cleanly with my changes. Sysinfo is still outdated and the optix wrapper should have been removed. Can you try updating your branch again?

@trigpolynom
Copy link
Contributor Author

Hmmm my branch is borked after messing with a reset. Do you want to open up a pr with your branch since you got path tracer working? I can close this one.

@jorge-ortega
Copy link
Collaborator

Closing this one and continuing in #171.

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.

3 participants