Implement Clippy

[kanerogers]

Description

Clippy will probably get mad.

TODO

• Incorporate Clippy
• Too Many Arguments problems
• Non Send Fields in Send Type problem
• Type Complexity Problems
• Remove commented references to Clippy in code

Problems

Clippy: Too Many Arguments

1. panel.rs add_panel_to_world (10/7)

  --> hotham/src/components/panel.rs:79:1                                                                                                                                                                       
   |                                                                                                                                                                                                            
79 | / pub fn add_panel_to_world(                                                                                                                                                                               
80 | |     text: &str,                                                                                                                                                                                          
81 | |     width: u32,                                                                                                                                                                                          
82 | |     height: u32,                                                                                                                                                                                         
...  |                                                                                                                                                                                                          
89 | |     world: &mut World,                                                                                                                                                                                   
90 | | ) -> Entity {                                                                                                                                                                                            
   | |___________^                                                                                                                                                                                              
   |                                                                                                                                                                                                            

2. gltf_loader.rs load_node (8/7)

  --> hotham/src/gltf_loader.rs:88:1                                                                                                                                                                            
   |                                                                                                                                                                                                            
88 | / fn load_node(                                                                                                                                                                                            
89 | |     node_data: &gltf::Node,                                                                                                                                                                              
90 | |     gltf_buffer: &[u8],                                                                                                                                                                                  
91 | |     vulkan_context: &VulkanContext,                                                                                                                                                                      
...  |                                                                                                                                                                                                          
96 | |     images: &[gltf::image::Data],                                                                                                                                                                        
97 | | ) -> Result<()> {                                                                                                                                                                                        
   | |_______________^                                                                                                                                                                                          
   |                                                                                                                                                                                                            

3. image.rs new (8/7)

warning: this function has too many arguments (8/7)                                                                                                                                                             
  --> hotham/src/image.rs:17:5                                                                                                                                                                                  
   |                                                                                                                                                                                                            
17 | /     pub fn new(                                                                                                                                                                                          
18 | |         handle: vk::Image,                                                                                                                                                                               
19 | |         view: vk::ImageView,                                                                                                                                                                             
20 | |         device_memory: vk::DeviceMemory,                                                                                                                                                                 
...  |                                                                                                                                                                                                          
25 | |         layer_count: u32,                                                                                                                                                                                
26 | |     ) -> Self {                                                                                                                                                                                          
   | |_____________^                                                                                                                                                                                            
   |                                                                                                                                                                                                            

4. vulkan_context.rs create_texture_image (9/7)

   --> hotham/src/resources/vulkan_context.rs:467:5                                                                                                                                                          
    |                                                                                                                                                                                                           
467 | /     pub fn create_texture_image(                                                                                                                                                                        
468 | |         &self,                                                                                                                                                                                          
469 | |         name: &str,                                                                                                                                                                                     
470 | |         image_buf: &[u8], // Clippy &Vec<u8>, ptr_arg for texture.rs                                                                                                                                    
...   |                                                                                                                                                                                                         
476 | |         offsets: Vec<vk::DeviceSize>,
477 | |     ) -> Result<(Image, vk::Sampler)> {
    | |_____________________________________^
    |

5. vulkan_context.rs create_textures_descriptor_sets (8/7)

   --> hotham/src/resources/vulkan_context.rs:802:5
    |
802 | /     pub fn create_textures_descriptor_sets(
803 | |         &self,
804 | |         set_layout: vk::DescriptorSetLayout,
805 | |         material_name: &str,
...   |
810 | |         emissive_map: &Texture,
811 | |     ) -> VkResult<Vec<vk::DescriptorSet>> {
    | |_________________________________________^

Clippy: Type Complexity

warning: very complex type used. Consider factoring parts into type definitions

1. texture.rs

   --> hotham/src/texture.rs:183:6
    |
183 | ) -> Result<(Vec<u8>, u32, u32, vk::Format, u32, u32, Vec<vk::DeviceSize>)> {
    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |

2. vertex.rs

  --> hotham/src/vertex.rs:35:12
   |
35 |           t: (
   |  ____________^
36 | |             Vector3<f32>,
37 | |             Vector3<f32>,
38 | |             Vector2<f32>,
...  |
41 | |             Vector4<f32>,
42 | |         ),
   | |_________^
   |

3. mod.rs

  --> examples/crab-saber/src/systems/mod.rs:15:9
   |
15 |         PreparedQuery<With<Visible, With<Cube, (&'a Colour, &'a RigidBody, &'a Collider)>>>,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

Clippy: Non Send Fields in Send Type

1. audio_context.rs impl Send for AudioContext

warning: this implementation is unsound, as some fields in `AudioContext` are `!Send`                                                                                                                           
  --> hotham/src/resources/audio_context.rs:97:1                                                                                                                                                                
   |                                                                                                                                                                                                            
97 | unsafe impl Send for AudioContext {}                                                                                                                                                                       
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                                       
   |                                                                                                                                                                                                            
   = note: `#[warn(clippy::non_send_fields_in_send_ty)]` on by default                                                                                                                                          
note: the type of field `stream` is `!Send`                                                                                                                                                                     
  --> hotham/src/resources/audio_context.rs:22:5                                                                                                                                                                
   |                                                                                                                                                                                                            
22 |     pub stream: Arc<Mutex<Stream>>,                                                                                                                                                                        
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                                                                         
   = help: use a thread-safe type that implements `Send`                                                                                                                                                        

[davidkuhta]

Probably good form to get clippy into the mix, I can help with this one.

[davidkuhta]

First cut on issue-59 branch, documented remaining errors above

[kanerogers]

Nice one, @davidkuhta!

How would you like to approach the remaining ones?

[davidkuhta]

@kanerogers How about just talk through them as I had a few thoughts on each.

TMA 1

It's only used for initializing GameContext, how about passing it the engine given that's in a sense how we use it, and then pulling the contexts out in the function? I think for those add functions it might be a cleaner interface?

[davidkuhta]

TMA 2

It seems like there's an element of load, spawn, and insert for each node, with the latter two aspects being involved with the world. What if we broke it down into some variant of those functions

[davidkuhta]

TMA 3

Given that this is a simple new method and unless there is a way/reason to simplify the struct, I think this would be an appropriate candidate to suppress the clippy warning

[davidkuhta]

TMA 4

I think we should pass create_texture_image an Image given that it's acting as a pass through and calling create_image to generate one internally. That would get rid of nearly half the arguments and simplify the logic.

[davidkuhta]

TMA 5

Lot's of unsafe in this so I suggest suppressing the clippy warning and backlogging it.

[davidkuhta]

TC 1

Definitely feels like there's an opportunity for those items to be captured in a Image related struct. ImageSpecs or something is what first comes to mind. That function is also only called once and in the same module, so there are likely other ways to simplify.

[davidkuhta]

TC 2

So 2 thoughts here, from the docs it appears izip! is deprecated so we should look at that. Maybe this function would be a good candidate for a warning suppress so it can be pulled into its own issue?

[davidkuhta]

TC 3

Not sure if there's anything that could be done here, as it seems to be hecs API related. Clippy warning suppression candidate?

[davidkuhta]

NSFiST 1

Suggest warning suppress and then put separate issue, given the nuances about Send and some of the audio discussions I recalled on discord.

[kanerogers]

2. 7)

Solution: bundle up Panel related props (eg. leave VulkanContext, etc. as is)

[kanerogers]

TMA 2

It seems like there's an element of load, spawn, and insert for each node, with the latter two aspects being involved with the world. What if we broke it down into some variant of those functions

Solution: split up the three tasks:

1. Build up node_entity_map
2. Add mesh
3. Add children

[kanerogers]

TMA 3

Given that this is a simple new method and unless there is a way/reason to simplify the struct, I think this would be an appropriate candidate to suppress the clippy warning

Solution: agreed, let's just suppress this for now

[kanerogers]

TMA 4

I think we should pass create_texture_image an Image given that it's acting as a pass through and calling create_image to generate one internally. That would get rid of nearly half the arguments and simplify the logic.

Solution: I think your proposed solution is good; let me just investigate this further and get back to you

[kanerogers]

TMA 5

Lot's of unsafe in this so I suggest suppressing the clippy warning and backlogging it.

Solution: wrap the textures in a struct like PBRTextures

[kanerogers]

TC 1

Definitely feels like there's an opportunity for those items to be captured in a Image related struct. ImageSpecs or something is what first comes to mind. That function is also only called once and in the same module, so there are likely other ways to simplify.

Solution: I think the return type is an Image? If not, it probably should be!

[kanerogers]

TC 2

So 2 thoughts here, from the docs it appears izip! is deprecated so we should look at that. Maybe this function would be a good candidate for a warning suppress so it can be pulled into its own issue?

Solution: split this out into its own issue

[kanerogers]

TC 3

Not sure if there's anything that could be done here, as it seems to be hecs API related. Clippy warning suppression candidate?

Solution: whisper words of wisdom / let it be

[kanerogers]

stream

Solution: this may have been a solution to appease Legion. Since we're not using it anymore, we can drop the Send implementation and indeed not wrap Stream in Arc<Mutex<T>>

[kanerogers]

@davidkuhta Just took a look at the stuff re: joint_matrices. That one is a bit more hairy - maybe we can split those ones out into a "Clippy Part Two" issue - would be great to at least get these low hanging fruits picked off today if we can! :D