Feat: loading xyz files by GeigerJ2 · Pull Request #17 · rs4rse/vizmat

GeigerJ2 · 2025-11-06T15:35:57Z

No description provided.

unkcpz

give my first go. This is actually the first time I review others rust code in github, thanks bro ;)

Two major requests:

can you check all pub keywords to see if all of them can be pub(crate) or even just private?
After adding this load_file functionality, I believe it is useless to have a default molecule stay there at beginning. I'd prefer to have an empty screen with the text you add and a button says "load the default structure". In the future, we can extend this button into some "example structure assets", but that is out of scope of this PR.

unkcpz · 2025-11-06T19:33:18Z

@@ -1,9 +1,9 @@
+// parse.rs


Suggested change

// parse.rs

unkcpz · 2025-11-06T19:33:43Z

 // Function to parse XYZ file format from string content
-#[allow(dead_code)]
-fn parse_xyz_content(contents: &str) -> Result<Crystal> {
+pub fn parse_xyz_content(contents: &str) -> Result<Crystal> {


Suggested change

pub fn parse_xyz_content(contents: &str) -> Result<Crystal> {

pub(crate) fn parse_xyz_content(contents: &str) -> Result<Crystal> {

unkcpz · 2025-11-06T19:37:56Z

+
+// Function to read XYZ file from path
+#[allow(dead_code)]
+pub fn read_xyz_file(path: &str) -> Result<Crystal> {
+    let contents =
+        std::fs::read_to_string(path).context(format!("Failed to read XYZ file: {}", path))?;
+    parse_xyz_content(&contents)
+}


Suggested change

// Function to read XYZ file from path

#[allow(dead_code)]

pub fn read_xyz_file(path: &str) -> Result<Crystal> {

let contents =

std::fs::read_to_string(path).context(format!("Failed to read XYZ file: {}", path))?;

parse_xyz_content(&contents)

}

not used anywhere, correct?

unkcpz · 2025-11-06T19:40:52Z

+pub(crate) struct FileUploadText;
+
+// System to set up file upload UI
+pub fn setup_file_ui(mut commands: Commands) {


Suggested change

pub fn setup_file_ui(mut commands: Commands) {

pub(crate) fn setup_file_ui(mut commands: Commands) {

unkcpz · 2025-11-06T19:52:38Z

+#[allow(dead_code)]
+pub fn clear_old_atoms(mut commands: Commands, atom_query: Query<Entity, With<AtomEntity>>) {


this method is not called?

unkcpz · 2025-11-06T20:17:30Z

+
+// System to update file upload UI
+pub fn update_file_ui(
+    file_drag_drop: Res<crate::io::FileDragDrop>,


I'll put use crate::io::FileDragDrop in the beginning of the file.

unkcpz · 2025-11-06T20:40:55Z

+            Transform::from_xyz(atom.x, atom.y, atom.z)
+                .with_scale(Vec3::splat(get_element_size(&atom.element))),


Nice, builder pattern.

unkcpz · 2025-11-06T20:42:43Z

                    ..default()
                },
-                Transform::default(), // inherit camera rotation; light points along -Z in local space
+                Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_4)),


Suggested change

Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_4)),

Transform::from_rotation(Quat::from_rotation_x(-f32::consts::FRAC_PI_4)),

Actually, after play with other molecules, I think we should not change this in this PR. the rotation of the light is a bit too much and make the shadow looks very distract. I'll focus on functionality and leave the ui not change for the moment.

unkcpz · 2025-11-07T09:50:56Z

+                load_default_crystal,
+                setup_scene,
+                setup_camera,
+                setup_file_ui,


You remove the .chain() thus it can not guarantee the crystal exist before the setup_scene which need a crystal. My suggestion is not have any crystal loaded at the beginning but

put a text showing to user that they can upload a file by drag and drop as you did.

adding a button to click and load the default molecule (water). We can then extend this to a dropdown or "what is your lucky crystal today" just like google's "I'm feeling lucky".

unkcpz · 2025-11-07T10:02:48Z

+
+                if let Some(extension) = path_buf.extension() {
+                    if extension == "xyz" {
+                        file_drag_drop.dragged_file = Some(path_buf.clone());


the .clone() needed here because you use it later in the HoveredFile match branch. However, I am not sure the println there actually print anything in the screen, it might be just print to console. But I'm fine with this, it is just prototype and a fun project, when it get more mature, we can go through all .clone() and audit with having performance under consideration.

unkcpz · 2025-11-07T10:16:59Z

+                commands.insert_resource(crystal.clone());
+                println!("Crystal updated with {} atoms", crystal.atoms.len());


there is a critical design decision we need to think about soon, that is how we store the crystal and make it cross the bevy world and our back-end crystal operations.

we keep a single one?

we make clone every time when new crystal appears and make a clone of it to let it into bevy world.

For now, it is just inter-op with file, clone is a nice solution. Thanks for bring this up.

unkcpz · 2025-11-25T16:19:39Z

If it is hard to rebase, you can leave it to me, there are quite some conflict.

unkcpz · 2025-11-26T14:38:07Z

Okay, I think I screw it up, I push wrong branch to your branch, and since it has no change to main, it close this pr automatically. I cannot push more to this branch. Can you reopen? (maybe clone again because your fork have the old name "vizcrystal").

Sorry.. But I can open a new pr as well, with your commits, I'll do that.

GeigerJ2 · 2025-12-03T19:58:24Z

Bru, I don't check here for a few days, and you are making this mess 💔 😆

unkcpz · 2025-12-04T21:53:32Z

yep, my skill issue, I am always not very good when rebase things with conflicts. Anyway, #28 start shaping, and I'll find time to polish it.

unkcpz requested changes Nov 6, 2025

View reviewed changes

unkcpz reviewed Nov 7, 2025

View reviewed changes

unkcpz closed this Nov 26, 2025

unkcpz force-pushed the feat/xyz-loading branch from 0cb62b6 to 7e9dddd Compare November 26, 2025 14:28

unkcpz mentioned this pull request Nov 26, 2025

Superseded pr17: drag-drop new structure #28

Merged

	pub fn parse_xyz_content(contents: &str) -> Result<Crystal> {
	pub(crate) fn parse_xyz_content(contents: &str) -> Result<Crystal> {

	pub fn setup_file_ui(mut commands: Commands) {
	pub(crate) fn setup_file_ui(mut commands: Commands) {

		#[allow(dead_code)]
		pub fn clear_old_atoms(mut commands: Commands, atom_query: Query<Entity, With<AtomEntity>>) {

		Transform::from_xyz(atom.x, atom.y, atom.z)
		.with_scale(Vec3::splat(get_element_size(&atom.element))),

	Transform::from_rotation(Quat::from_rotation_x(-std::f32::consts::FRAC_PI_4)),
	Transform::from_rotation(Quat::from_rotation_x(-f32::consts::FRAC_PI_4)),

		commands.insert_resource(crystal.clone());
		println!("Crystal updated with {} atoms", crystal.atoms.len());

Conversation

GeigerJ2 commented Nov 6, 2025

Uh oh!

unkcpz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkcpz Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unkcpz commented Nov 25, 2025

Uh oh!

unkcpz commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeigerJ2 commented Dec 3, 2025

Uh oh!

unkcpz commented Dec 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

unkcpz Nov 6, 2025 •

edited

Loading

unkcpz commented Nov 26, 2025 •

edited

Loading