CylonSB / bounded-planet

Test Game Please Ignore
6 stars 1 forks source link

Feature/land mesh2 #33

Closed martindevans closed 4 years ago

martindevans commented 4 years ago

Main change is that the landscape now has normal data (Resolve #27).

Also some other changes that will make implementing #31 (my next task) easier:

martindevans commented 4 years ago

Clippy was failing due to one warning (many_single_char_names), I've added a longer name to satisfy clippy but this warning will need some reconfiguring in the future.

Jerald commented 4 years ago

Having raw numbers exposed in an api, especially an external one, is somewhat scary to me. It's relatively brittle and lends itself to accidentally misusing things.

martindevans commented 4 years ago

raw numbers ... public API

The main API surface is:

let wrap = TextureHeightmap::new(textures.get(&land_texture_handle).expect("Couldn't get texture")).expect("Couldn't wrap texture");
let land_mesh = texture_to_mesh(&wrap).expect("Couldn't turn texture to mesh");

HeightmapData (the main place numbers are exposed) is ultimately only really going to be used within the mesh generation system.

Jerald commented 4 years ago

I was mostly referring to the fact that the structures themselves are marked as pub and are therefore accessible externally. Either way, we can leave things as they are now. If issues come up, I think we know a place to possibly put some work.

martindevans commented 4 years ago
main_2020-09-29_00-47-38
martindevans commented 4 years ago

I was mostly referring to the fact that the structures themselves are marked as pub

Ideally the entire landscape meshing system would be contained within a separate crate to the rest of the game and the HeightmapData trait will be an implementation detail. I'll be doing more work on more advanced mesh generation in a followup PR (after we've changed to a workspace) so I might see if I can rearrange things to be nicer then.

Nyefari commented 4 years ago

The only weird thing I had noticed before you've already fixed. Should have submitted the review yesterday, sorry.