shinzlet / atomCAD

Mozilla Public License 2.0
0 stars 0 forks source link

Not all atoms render properly #1

Open shinzlet opened 1 year ago

shinzlet commented 1 year ago

While hardcoding a simple part, I found that there was some unexpected rendering behaviour.

shinzlet commented 1 year ago

on bce2534, the following is rendered for the part described by:

vec![
    AtomRepr {
        pos: Vec3 {
            x: 0.0,
            y: 0.0,
            z: 0.0,
        },
        kind: AtomKind::new(Element::Carbon),
    },
    AtomRepr {
        pos: Vec3 {
            x: 5.0,
            y: 0.0,
            z: 0.0,
        },
        kind: AtomKind::new(Element::Sulfur),
    },
]
Screen Shot 2023-07-18 at 3 19 07 PM

notice how only one atom is rendered, even though two are specified. If the coordinates are switched, the behaviour is even weirder:

vec![
    AtomRepr {
        pos: Vec3 {
            x: 5.0, // in the last code block this was 0.0
            y: 0.0,
            z: 0.0,
        },
        kind: AtomKind::new(Element::Carbon),
    },
    AtomRepr {
        pos: Vec3 {
            x: 0.0, // in the last code block this was 5.0
            y: 0.0,
            z: 0.0,
        },
        kind: AtomKind::new(Element::Sulfur),
    },
]
Screen Shot 2023-07-18 at 3 21 12 PM

Now that the coordinates are switched, two atoms render in roughly the correct position. But it looks like it is being drawn as hydrogen, not sulfur.

This led me to believe that the final atom's data is being ignored entirely, and this is easy to verify. changing the model to:

vec![
    AtomRepr {
        pos: Vec3 {
            x: 5.0,
            y: 0.0,
            z: 0.0,
        },
        kind: AtomKind::new(Element::Carbon),
    },
    AtomRepr {
        // This should move the atom offscreen!
        pos: Vec3 {
            x: 1000.0,
            y: 1000.0,
            z: 1000.0,
        },
        kind: AtomKind::new(Element::Sulfur),
    },
]

does not cause the "fake hydrogen" to move, indicating that the x, y, z, and kind fields of that AtomRepr aren't being used by the render pass.

As a final check, I locally edited the periodic_table crate to make Hydrogen render in red. This confirmed that the renderer thinks this ghost atom is hydrogen:

Screen Shot 2023-07-18 at 3 28 44 PM
shinzlet commented 1 year ago

This led me to believe that an off-by-one error is responsible - the gpu knows how many atoms it should render, but the actual Atoms buffer is being truncated too short (and the rest of the memory is zeroed out, leading to a hydrogen (element 0 in our lookup table) being placed at <0.0, 0.0, 0.0>. I wrote a quick fix for this in 1af14ce, and rendering seems to be fixed due to it. I'm still including this writeup and keeping that fix in a separate branch, though, as I found it by trial and error rather. I think that this issue may be introduced by the atom buffer header, so it's possible I should change 0..((fragment.atoms().len() + 1) * 3).try_into().unwrap() into 0..(size of the atom buffer header + (fragment.atoms().len() * 3)).try_into().unwrap().

shinzlet commented 1 year ago

This is caused because of some code in atoms.rs: https://github.com/shinzlet/atomCAD/blob/cc4be77181b184cae079fd680cc36e9dbdea98d4/crates/render/src/atoms.rs#L97C35-L97C35

The buffer offset is hardcoded to 0, but the atoms buffer has a header whose size can be nonzero. Unfortunately, offset has to be a multiple of the min_storage_buffer_offset_alignment of the GPU (256 bytes by default). So, this bug can be fixed, it just requires making the header a fair bit larger. Atoms is only uploaded to the GPU once per molecule, though, so I don't foresee a performance impact.