Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gui #227

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Gui #227

wants to merge 5 commits into from

Conversation

cedric-h
Copy link

@cedric-h cedric-h commented Jun 26, 2019

I tried to run cargo fmt, but that didn't seem to work at all.
I think that's why almost every file was changed. I saw your .rustfmt, so I figured it was configured the way you guys wanted it.
The files you should be looking at, then, are src/window.rs, src/gui.rs, and examples/gui.rs.
I broke backwards compatibility because of the Window struct, right now for all of the other examples you'd have to do Window<gui::NoBackend>::new() to make a new window.
I'm not sure how you'd want me to fix that, whether you'd want me to cfg out everything GUI related, or maybe make what is now the window into GuiWindow, and then typedef Window as GuiWindow<NoBackend>. Either of these and several other things could preserve backwards compatibility, but it's up to you guys.


This change is Reviewable

@cedric-h
Copy link
Author

Naturally my PR doesn't pass the tests, but that's because of the window instantiation backwards compat thing, like I explained above.

Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 51 files at r1, 1 of 3 files at r2.
Reviewable status: 6 of 51 files reviewed, 2 unresolved discussions (waiting on @cedric-h)


Cargo.toml, line 34 at r2 (raw file):

genmesh = "0.6"
gfx = "0.17.1"
gfx_core = "^0.8"

this is unexpected - all the required types should have been re-exported by gfx crate iself


examples/anim.rs, line 86 at r2 (raw file):

    cam.look_at([100.0, 0.0, 100.0], [0.0, 0.0, 30.0], Some([0.0, 1.0, 0.0].into()));

    let geom = three::Geometry { base: three::Shape { vertices: make_vertices(VERTICES), normals: Vec::new(), ..three::Shape::default() }, faces: make_indices(INDICES), shapes: V_FLY.iter().map(|data| three::Shape { vertices: make_vertices(data), ..three::Shape::default() }).collect(), ..three::Geometry::default() };

this looks somewhat sad
Could you make a PR without formatting any existing code?

@ecton
Copy link

ecton commented Mar 5, 2020

@kvark Hey, I'm loving this library and was looking into updating the Cargo.toml to get onto current versions of libraries, and in the process of fixing some of that, the rustfmt.toml issue that I found on gitter came up as I edited factory/mod.rb. In current versions of toolchains, this file shrinks drastically because max_width now appears to transform all multiline expressions, such as the one you mentioned in the last comment on here.

What is your current opinion on how the formatting of files for this project should happen? I'd suggest removing the max_width directive, and using #[rustfmt::skip] in places where you want to control the formatting specifically -- I'm guessing mostly the data arrays?

Sorry for hijacking this PR for the question -- it seemed relevant to this issue's conversation. Great library, btw!

@kvark
Copy link
Collaborator

kvark commented Mar 5, 2020

@ecton this is indeed unrelated to the PR:) Please open an issue about rust formatting (or a PR with the suggest changes). Roughly, your proposal looks reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants