FraserLee / bevy_sprite3d

Use sprites in a 3d bevy scene.
MIT License
144 stars 22 forks source link

Simple cleanup #11

Closed dmyyy closed 1 year ago

dmyyy commented 1 year ago
FraserLee commented 1 year ago

First of all, thanks so much for the interest in the project! I really hope this doesn't come off as too harsh.

I have significant philosophical disagreements with the idea of blanket applying formatting rules across a codebase. For example the change

     let mut map = vec![
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), tl(),  t(),   d(),   d(),   d(),   t(),   tr() ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   f(),   f(),   f(),   f(),   r()  ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), d(),   f(),   d(),   d(),   d(),   f(),   d()  ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), d(),   f(),   d(),   d(),   d(),   f(),   d()  ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), d(),   f(),   d(),   d(),   d(),   f(),   d()  ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   f(),   f(),   f(),   f(),   r()  ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), bl(),  b(), (8,21),  d(), (8,22),  b(),   br() ],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   r(),   (0,0), (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   d(),   r(),   (0,0), (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), tl(), (8,19), f(),  (8,20), tr(),  (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   d(),   f(),   r(),   (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   f(),   f(),   r(),   (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   d(),   f(),   r(),   (0,0)],
-        vec![(0,0), (0,0), (0,0), (0,0), (0,0), (0,0), l(),   f(),   f(),   f(),   r(),   (0,0)],
-        vec![tl(),  t(),    tr(), (0,0), (0,0), (0,0), l(),   f(),   f(),   f(),   r(),   (0,0)],
-        vec![l(),   f(),  (8,25),  tb(),  tb(),  tb(), (8,24),f(),   f(),   f(),   r(),   (0,0)],
-        vec![bl(),  b(),    br(), (0,0), (0,0), (0,0), bl(),  b(),   b(),   b(),   br(),  (0,0)],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            tl(),
+            t(),
+            d(),
+            d(),
+            d(),
+            t(),
+            tr(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            l(),
+            f(),
+            f(),
+            f(),
+            f(),
+            f(),
+            r(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            d(),
+            f(),
+            d(),
+            d(),
+            d(),
+            f(),
+            d(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            d(),
+            f(),
+            d(),
+            d(),
+            d(),
+            f(),
+            d(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            d(),
+            f(),
+            d(),
+            d(),
+            d(),
+            f(),
+            d(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            l(),
+            f(),
+            f(),
+            f(),
+            f(),
+            f(),
+            r(),
+        ],
+        vec![
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            (0, 0),
+            bl(),
+            b(),
+            (8, 21),
+            d(),
+            (8, 22),
...

makes the code extraordinarily less readable. There is semantic meaning (the fact that it's a 2d map) that's lost when applying a formatter. Even simpler changes like

-enum GameState { Loading, Ready }
+enum GameState {
+    Loading,
+    Ready,
+}

end up with a less readable file, where you can fit less on the screen. Beyond dogma, there's no reason a 2 member enum shouldn't be one line. Readability matters, and changes like these

 // everything needed to register a sprite, passed in one go.
 #[derive(SystemParam)]
 pub struct Sprite3dParams<'w, 's> {
-    pub meshes    : ResMut<'w, Assets<Mesh>>,
-    pub materials : ResMut<'w, Assets<StandardMaterial>>,
-    pub images    : ResMut<'w, Assets<Image>>,
-    pub atlases   : ResMut<'w, Assets<TextureAtlas>>,
-    pub sr        : ResMut<'w, Sprite3dRes>,
+    pub meshes: ResMut<'w, Assets<Mesh>>,
+    pub materials: ResMut<'w, Assets<StandardMaterial>>,
+    pub images: ResMut<'w, Assets<Image>>,
+    pub atlases: ResMut<'w, Assets<TextureAtlas>>,
+    pub sr: ResMut<'w, Sprite3dRes>,
     #[system_param(ignore)]
     marker: PhantomData<&'s usize>,
 }

make the codebase worse.

Other changes - like the presence of "excess" newlines or spaces - visually define boundaries between different ideas in the code. The cumulative effect of paving over all those nuances is to strip meaning, ending with a less expressive less communicative piece of code.

The colour vs color thing is far more arbitrary, but given it's neutral either way I'd rather we fight back against US defaultism online \:)

If you have specific formatting changes you would like to make with specific justifications (for example, moving the imports to the top of the library), feel free to open another PR - but I'm going to close this one. Thanks again for the interest!

dmyyy commented 1 year ago

US defaultism

That's fair we can leave that as is

significant philosophical disagreements with the idea of blanket applying formatting rules across a codebase

I think there's value to using the same formatting standard across the rust ecosystem (readability, ease of understanding) and the formatting in this lib is an outlier from any other rust repo I've looked at. IMO the unique takes on where to add spaces and arbitrary choices across the board may make it easier for you to understand your code but it doesn't make it easier for the average bevy user (like me).

I tried opening another pr just with clippy fixes but I can't - I need to go out of my way to disable formatting in my ide in order to contribute.

I urge you to consider formatting the code the same way bevy does. Have a good day!

FraserLee commented 1 year ago

Fair enough. If I have some time, I'll investigate writing a rustfmt.toml that's less destructive