Add navigation cube #1573

erenoku · 2023-02-09T16:42:18Z

I opened this PR as a draft as I have some questions.

The code structure. Should the navigation_cube.rs file be located in the folder named graphics or should a new folder be created for this and the other files like model.rs which handles the loading of models. Btw most of the boilerplate handling the loading of the model, textures and meshes are copied from learn-wgpu's github repository.
The textures and the model in the assests directory should somehow be bundled with the application when installing. I don't think this is possible without an installer script as these files should somehow be downloaded into a specific directory in the users computer.
We might want to use a perspective camera to render the cube. In spite of my tries for some reason I could not get it working and decided to use an orthographic camera. Although this not a huge problem as the cube is really small and it is almost impossible to notice anything seeming off it would be good if this was fixed before merging.

Here is a photo showing how it looks.

Closes #15

hannobraun

Thank you, @erenoku, this is awesome! #15 is one of the oldest open issues we have in this repository, and it's great that you're addressing it!

I've only taken a cursory look at your code, as I don't know how ready for review it is. My priority was to answer your questions, but I left comments where things stood out to me.

The code structure. Should the navigation_cube.rs file be located in the folder named graphics or should a new folder be created for this and the other files like model.rs which handles the loading of models.

Since that code seems to use wgpu heavily, I think graphics is the appropriate place to put it.

In general, I think these kinds of code structure issues are not critical. If you're unsure, just choose whatever works. If it becomes clear later that another structure is better, we can always change it.

The textures and the model in the assests directory should somehow be bundled with the application when installing. I don't think this is possible without an installer script as these files should somehow be downloaded into a specific directory in the users computer.

Can we instead bundle them directly into the binary using include_bytes!?

We might want to use a perspective camera to render the cube. In spite of my tries for some reason I could not get it working and decided to use an orthographic camera. Although this not a huge problem as the cube is really small and it is almost impossible to notice anything seeming off it would be good if this was fixed before merging.

I have a large screen in my office, and the cube does look really weird 😄

But don't sweat it. If you come up with a solution before merging, then great. But having the navigation cube at all is such a great improvement that having it look a bit weird is not a big problem. We can just open an issue after merging and leave fixing it for later.

Other notes:

Since the assets are used within fj-viewer, wouldn't it be more appropriate to move the directory to crates/fj-viewer?
If I had done this, I would have tried a different approach: Create a cube "manually" (by specifying vertices and indices directly in the code) and render the text on it programmatically (using egui, if possible; or using whatever text rendering library egui uses, if not). I don't know if that would actually be better, and I'm definitely not asking you to change it! I just wanted to mention this approach as a possible future change, as it has the advantage of being more flexible and not requiring any external assets (beyond a font, which we already need to bundle anyway).

How close do you think this pull request is to being merge-ready? As I said above, I have looked at the code in detail yet, but it seems like the navigation cube works great as-is, and I'd like to merge it as soon as possible. The only real blocker I see is the bundling of assets. But if my include_bytes! suggestion works, that shouldn't be a problem.

hannobraun · 2023-02-10T10:48:19Z

crates/fj-viewer/Cargo.toml

@@ -11,18 +11,26 @@ keywords.workspace = true
 categories.workspace = true

 [dependencies]
+anyhow = "1.0.68"


This is only a small issue, but Anyhow isn't really the appropriate library to use here, as it's optimized for applications, not for libraries where users might want to handle the specific errors differently. The better choice is its sibling library, thiserror, which fj-viewer already depends on (aee Anyhow README).

erenoku · 2023-02-11T17:19:20Z

Thank you for the review.

I think computing the vertices of the cube manually isn't really a good idea as the cube.obj file is just a 1130 byte file that stores the vertices and the indices that we would normally calculate and storing the navigation cube as a model would allow us to easily create more complex cube designs for example with chamfered edges and corners to allow the user to change the camera angle. But I found the idea of writing the text programmatically a good idea and it would also allow us get rid of embedding the png files into the binary and make it possible to implement translation to other languages.

Using include_bytes!() to embed the assets into the binary also came to my mind but I didn't want to do it as it would result in a hard-coded code like this and the model loading code would only work for the navigation cube.

let texture_data: &[u8] = match m.diffuse_texture.as_str() {
    "front.png" => {
        include_bytes!("../../../../assets/navigation_cube/front.png")
    }
    "right.png" => {
        include_bytes!("../../../../assets/navigation_cube/right.png")
    }
    // and so on

    _ => return LoadModelError::MaterialNotFound,
};

But as I will try to render the text programmatically this won't be a problem and only embedding cube.obj is not a big deal.

I am not completely sure how sure how to render the text using egui so I cannot comment on how long it will take before attempting to do it. But if you want to merge this PR as soon as possible the code should be quickly merge-ready after I change the code to load the textures using include_bytes!() and use thiserror instead of anyhow. Then using a perspective camera and rendering the text using egui could be implemented later.

hannobraun · 2023-02-11T17:58:42Z

I think computing the vertices of the cube manually isn't really a good idea as the cube.obj file is just a 1130 byte file that stores the vertices and the indices that we would normally calculate and storing the navigation cube as a model would allow us to easily create more complex cube designs for example with chamfered edges and corners to allow the user to change the camera angle.

That makes sense 👍

But I found the idea of writing the text programmatically a good idea and it would also allow us get rid of embedding the png files into the binary and make it possible to implement translation to other languages.

Also makes sense! Mixing both approaches, as you suggest, might be the best option.

I am not completely sure how sure how to render the text using egui so I cannot comment on how long it will take before attempting to do it.

I honestly don't know if using egui for this makes any sense. I think it should be practical, as egui just uses whatever render pass we give it. So I don't see a reason why we couldn't just render it to a texture and draw it on the cube. But I might be missing something, since I'm a total wgpu/egui amateur.

If egui causes trouble, we can use whatever text rendering library egui uses under the hood to render the text instead. (Or any other library, really, but using the one egui already uses means we don't need an additional dependency.) I have a secret hope though that makes me want to use egui to draw on the cube: maybe we can draw interactive UI elements on the cube. That would require some transform magic, but maybe it's doable.

But in any case, that's just me dreaming. If egui works for this, fine. If not, using something else (or staying with the PNGs) is totally acceptable.

But if you want to merge this PR as soon as possible the code should be quickly merge-ready after I change the code to load the textures using include_bytes!() and use thiserror instead of anyhow. Then using a perspective camera and rendering the text using egui could be implemented later.

Yeah, I'd like to merge this as soon as we can. Having the navigation cube at all is already such a great improvement. And pull requests that stay open for too long just collect merge conflicts, which causes extra work.

As I said before, the only real blocker are the assets, as we don't have the infrastructure (i.e. installers) right now to distribute them. (And even if we built that infrastructure, we'd permanently lose the ability to just cargo install fj-app, which would be a shame.) Anything else can be improved later.

erenoku · 2023-02-12T08:20:29Z

This PR should now be ready for merging. I will retry using a perspective projection later.

And even if we built that infrastructure, we'd permanently lose the ability to just cargo install fj-app, which would be a shame.

This is probably not relevant for now but if later bundling assets becomes necessary we can check every time the fj-app is started whether a specific directory in the users computer exists and if not start downloading the assets into that directory.

Edit: I managed to get perspective camera working.

hannobraun · 2023-02-12T18:43:06Z

Thank you, @erenoku! I'll take a look tomorrow.

hannobraun

Thanks again, @erenoku, this looks great!

I've left a few comments were I noticed potential problems, but none of those are critical, and none of those change that this is a great addition. I'm going to merge this pull request now.

Feel free to address any comments you find worth addressing in a follow-up pull request, if you want.

hannobraun · 2023-02-13T13:00:08Z

crates/fj-viewer/src/graphics/mod.rs

@@ -3,9 +3,12 @@
 mod draw_config;
 mod drawables;
 mod geometries;
+mod model;


I think the naming of this module and some of the types in it is not ideal. It makes sense when viewed in isolation, but in the larger context of Fornjot, it could be confusing. Someone might search for the code that draws CAD models, and then think they found it here.

It's not a critical issue, and I don't have any better ideas right now. Just making a note, hoping that someone will come with with something better at some point.

hannobraun · 2023-02-13T13:06:36Z

crates/fj-viewer/src/graphics/model.rs

+            let vertices = (0..m.mesh.positions.len() / 3)
+                .map(|i| ModelVertex {
+                    position: [
+                        m.mesh.positions[i * 3],
+                        m.mesh.positions[i * 3 + 1],
+                        m.mesh.positions[i * 3 + 2],
+                    ],
+                    tex_coords: [
+                        m.mesh.texcoords[i * 2],
+                        1.0 - m.mesh.texcoords[i * 2 + 1],
+                    ],
+                })


Those magic numbers here (specifically, the 3 used for vertex position and the 2 used for texture coordinates) aren't great. It's easy enough to figure out why they are here (number of vertices in a triangle and number of texture coordinates in 2 dimensions, respectively), but it gave me pause and looks a bit iffy.

It might make sense to extract them into appropriately named constants, and extract the i * 3/i * 2 into variables. Although I don't have ideas on what to name those.

Anyway, I'm nitpicking!

hannobraun · 2023-02-13T13:08:46Z

crates/fj-viewer/src/graphics/model.rs

+pub trait DrawModel<'a> {
+    fn draw_mesh(&mut self, mesh: &'a Mesh, material: &'a Material);
+    fn draw_mesh_instanced(
+        &mut self,
+        mesh: &'a Mesh,
+        material: &'a Material,
+        instances: Range<u32>,
+    );
+
+    fn draw_model(&mut self, model: &'a Model);
+    fn draw_model_instanced(&mut self, model: &'a Model, instances: Range<u32>);
+}


Having a trait and then implementing it for wgpu::RenderPass might be a bit too clever, and therefore potentially confusing. I think I would prefer just having functions that accept all the necessary parameters.

hannobraun · 2023-02-13T13:12:21Z

crates/fj-viewer/src/graphics/navigation_cube.rs

+const SCALE_FACTOR: f64 = 0.13;
+const CUBE_TRANSLATION: [f64; 3] = [0.8, 0.7, 0.0];


It seems those constants are used only in a single method. Maybe it makes more sense to move them into the local scope of that method. That would make their meaning a bit clearer, I think, as they would be closer to the code that uses them.

hannobraun · 2023-02-13T13:26:08Z

crates/fj-viewer/src/graphics/renderer.rs

+            &device,
+            &queue,
+            &surface_config,
+            800.0 / 600.0,


I don't understand why we need to pass an aspect ratio here, and why it is set to this value specifically.

Everything seems to work fine, so it's not too much of a problem, but I figure this could be made clearer. Since the aspect ratio is set to a constant anyway, maybe we should move this value to where it's actually used and add a comment there, explaining why that specific value was chosen.

hannobraun reviewed Feb 10, 2023

View reviewed changes

erenoku force-pushed the navigation_cube branch 2 times, most recently from e6e76d2 to e941184 Compare February 12, 2023 08:11

erenoku marked this pull request as ready for review February 12, 2023 08:11

hannobraun approved these changes Feb 13, 2023

View reviewed changes

erenoku added 14 commits February 13, 2023 14:30

rendering cube infastructure

0ffdf28

cube can rotate

cce6511

add translation add scale for the navigation cube

b09f840

fix mirrored texture

d0fedbd

fix aspect ratio of navigation cube changing when the window is resized

0335700

fix issues and add borders to the edges of the cube

9151e80

add navigation cube

ad3b426

minor change in documentation

095f12d

cleanup

e4e8e15

Fix new clippy lint

3f86c02

convert to using thiserror instead of anyhow

dc8476b

embed assets into the binary

cec4a37

Use a perspective camera to render the navigation cube

de6bd37

Rename model_matrix to mvp_matrix

533f6c2

hannobraun force-pushed the navigation_cube branch from eb2ad39 to 533f6c2 Compare February 13, 2023 13:30

hannobraun enabled auto-merge February 13, 2023 13:30

hannobraun merged commit 5879a0c into hannobraun:main Feb 13, 2023

erenoku deleted the navigation_cube branch February 13, 2023 16:55

hannobraun mentioned this pull request Feb 20, 2023

cargo install failed on 0.37.0 #1603

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add navigation cube #1573

Add navigation cube #1573

erenoku commented Feb 9, 2023

hannobraun left a comment

hannobraun Feb 10, 2023

erenoku commented Feb 11, 2023

hannobraun commented Feb 11, 2023

erenoku commented Feb 12, 2023 •

edited

Loading

hannobraun commented Feb 12, 2023

hannobraun left a comment

hannobraun Feb 13, 2023

hannobraun Feb 13, 2023

hannobraun Feb 13, 2023

hannobraun Feb 13, 2023

hannobraun Feb 13, 2023

		const SCALE_FACTOR: f64 = 0.13;
		const CUBE_TRANSLATION: [f64; 3] = [0.8, 0.7, 0.0];

Add navigation cube #1573

Add navigation cube #1573

Conversation

erenoku commented Feb 9, 2023

hannobraun left a comment

Choose a reason for hiding this comment

hannobraun Feb 10, 2023

Choose a reason for hiding this comment

erenoku commented Feb 11, 2023

hannobraun commented Feb 11, 2023

erenoku commented Feb 12, 2023 • edited Loading

hannobraun commented Feb 12, 2023

hannobraun left a comment

Choose a reason for hiding this comment

hannobraun Feb 13, 2023

Choose a reason for hiding this comment

hannobraun Feb 13, 2023

Choose a reason for hiding this comment

hannobraun Feb 13, 2023

Choose a reason for hiding this comment

hannobraun Feb 13, 2023

Choose a reason for hiding this comment

hannobraun Feb 13, 2023

Choose a reason for hiding this comment

erenoku commented Feb 12, 2023 •

edited

Loading