Triangle example using GLFW #1258

Haggus · 2017-05-08T21:15:56Z

GLFW triangle example from #1199

kvark · 2017-05-08T21:39:19Z

The problem here is minimizing the code duplication with existing code. This PR copies a lot of stuff (code, screenshot, shaders, etc). I wonder if we can have everything as optional code paths inside the same triangle example.

Haggus · 2017-05-08T22:09:33Z

Oh, I thought that was the point. To make the triangle example use glfw, and make it as close to glutin implementation as possible :)

By "optional code paths", do you mean having triangle examples share the same assets?

kvark · 2017-05-09T02:44:54Z

@Haggus I'm sorry, there is a lot of discussion context between me and @icefoxen that got lost and not expressed in the original issue. I believe there is a solution that would require minimum code/maintenance and would be helpful for anyone choosing GLFW/SDL. This solution is to:

Have the examples to be inside the corresponding window backend folders (src/window/glfw/examples/)
Don't draw anything, just clear the screen. We can have a comment referring to the triangle example for the actual drawing code, it's not going to be any different.

How does this sound to you?

Haggus · 2017-05-09T15:23:48Z

@kvark that sounds good. I don't mind changing it, so if this is how you guys want it, then it's OK with me. I'll submit changes as soon as I can!

Haggus · 2017-05-10T14:42:45Z

@kvark let me know what you think about these changes.

Also, now that I think about it, this example should probably be called "GLFW Example".

kvark

Thank you! We are almost there ;)

kvark · 2017-05-10T16:45:30Z

src/window/glfw/Cargo.toml

@@ -28,6 +28,11 @@ documentation = "https://docs.rs/gfx_window_glfw"
 name = "gfx_window_glfw"

 [dependencies]
-glfw = "0.13"
+glfw = "0.14"
+gfx = "0.15"


this should go to [dev-dependencies]

kvark · 2017-05-10T16:46:01Z

src/window/glfw/Cargo.toml

+
+[[example]]
+name = "window"
+path = "examples/window/main.rs"


let's simplify it by placing the example as examples/window.rs, so that you don't even need this path= line here

kvark · 2017-05-10T16:46:56Z

src/window/glfw/examples/window/main.rs

+    window.make_current();
+    glfw.set_error_callback(glfw::FAIL_ON_ERRORS);
+    let (_, _, _, _) = gfx_window_glfw::init(&mut window);
+


let's add a //Note: actual drawing code is no different from the triangle example, or any other.

kvark · 2017-05-10T16:47:05Z

src/window/glfw/examples/window/main.rs

+    glfw.window_hint(glfw::WindowHint::OpenGlForwardCompat(true));
+    glfw.window_hint(glfw::WindowHint::OpenGlProfile(glfw::OpenGlProfileHint::Core));
+
+    let (mut window, events) = glfw.create_window(1024, 768, "Triangle example", glfw::WindowMode::Windowed)


let's change the title here

Haggus · 2017-05-10T18:40:38Z

Should be good now. Let me know if you'd like any other changes! :)

kvark · 2017-05-10T18:53:28Z

Cargo.toml

@@ -145,4 +145,4 @@ image = "0.13"
 winit = "0.6"

 [target.x86_64-unknown-linux-gnu.dev-dependencies]
-glfw = "0.12"
+glfw = "0.14"


Actually, I don't know why it's even here? gfx_app doesn't use glfw or SDL anyway.
Let's remove it?

kvark · 2017-05-10T18:53:45Z

src/window/glfw/Cargo.toml

@@ -28,6 +28,9 @@ documentation = "https://docs.rs/gfx_window_glfw"
 name = "gfx_window_glfw"

 [dependencies]
-glfw = "0.13"
+glfw = "0.14"


this needs a version bump for this crate

Isn't 0.14 the latest version?

I mean, we should bump the version of gfx_window_glfw to 0.15, given that dependency change

kvark · 2017-05-10T18:54:05Z

src/window/glfw/examples/window.rs

@@ -0,0 +1,55 @@
+// Copyright 2015 The Gfx-rs Developers.


we are 2 years in the future, yay!

kvark · 2017-05-10T19:09:47Z

src/window/glfw/Cargo.toml

 gfx_core = { path = "../../core", version = "0.7" }
 gfx_device_gl = { path = "../../backend/gl", version = "0.14" }
+
+[dev-dependencies]
+gfx = "0.15"


actually, you need to provide both the local path and the version, similar to the dependencies above

kvark

beautiful, thank you!

kvark · 2017-05-10T19:51:59Z

src/window/glfw/Cargo.toml

@@ -28,6 +28,9 @@ documentation = "https://docs.rs/gfx_window_glfw"
 name = "gfx_window_glfw"

 [dependencies]
-glfw = "0.13"
+glfw = "0.14"


I mean, we should bump the version of gfx_window_glfw to 0.15, given that dependency change

kvark · 2017-05-10T19:52:36Z

src/window/glfw/Cargo.toml

 gfx_core = { path = "../../core", version = "0.7" }
 gfx_device_gl = { path = "../../backend/gl", version = "0.14" }
+
+[dev-dependencies]
+gfx = { path = "../../../src/render", version = "0.15" }


I'd assume you can go with "../../render". It still doesn't explain why travis fails to compile. Does it compile for you?

if I do "../../render", it doesn't compile for me. With "src" it does without issues.

I did "cargo clean", it recompiled and works.

kvark · 2017-05-10T20:08:25Z

There is one small fix needed in the root Cargo.toml now to point to gfx_window_glfw 0.15

Haggus · 2017-05-10T20:34:34Z

Should be good now :)

kvark · 2017-05-10T20:35:38Z

Thanks! 🤞

kvark · 2017-05-10T21:08:59Z

This looks like a bug :( I can repro it on my machine. cargo test -p gfx_window_glfw doesn't use dev-dependencies section for some reason. I propose to move it into the regular dependencies, but add a //TODO note mentioning the problem. Also, the path should really just be "../../render".

Haggus · 2017-05-10T21:14:54Z

Here is the bug: rust-lang/cargo#860

kvark · 2017-05-10T21:22:06Z

Thanks @Haggus for finding the issue! I commented on there.

1258: Lint all the things r=kvark a=kvark **Connections** Fixes gfx-rs#1250 **Description** Refactors the code to satisfy the linter, configures it, and enables on CI. **Testing** `cargo clippy` saves the dau Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

Triangle example using GLFW

ac794d0

Move glfw triangle example to glfw folder. Strip drawing code.

7138ffc

kvark requested changes May 10, 2017

View reviewed changes

Minor fixes to glfw window example

05b4a12

kvark reviewed May 10, 2017

View reviewed changes

Mateusz 'Haggus' Mrowiec added 2 commits May 10, 2017 21:30

Minor fixes

d2e6078

Update date on window example

dfce190

kvark approved these changes May 10, 2017

View reviewed changes

kvark reviewed May 10, 2017

View reviewed changes

Bump gfx_window_glfw to 0.15

bbacd21

Update dependency version

6f7e15c

Fix path

9a0953b

kvark mentioned this pull request May 10, 2017

Dev-dependencies are not linked when testing subpackages rust-lang/cargo#860

Closed

kvark merged commit 82f2878 into gfx-rs:master May 11, 2017

This was referenced May 15, 2017

Add sdl window example, update to 0.30 #1271

Merged

Window creation examples for SDL and GLFW #1199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Triangle example using GLFW #1258

Triangle example using GLFW #1258

Haggus commented May 8, 2017

kvark commented May 8, 2017

Haggus commented May 8, 2017

kvark commented May 9, 2017

Haggus commented May 9, 2017

Haggus commented May 10, 2017

kvark left a comment

kvark May 10, 2017

kvark May 10, 2017

kvark May 10, 2017

kvark May 10, 2017

Haggus commented May 10, 2017

kvark May 10, 2017

kvark May 10, 2017

Haggus May 10, 2017

kvark May 10, 2017

kvark May 10, 2017

kvark May 10, 2017

kvark left a comment

kvark May 10, 2017

kvark May 10, 2017

Haggus May 10, 2017

Haggus May 10, 2017

kvark commented May 10, 2017

Haggus commented May 10, 2017

kvark commented May 10, 2017

kvark commented May 10, 2017

Haggus commented May 10, 2017

kvark commented May 10, 2017

Triangle example using GLFW #1258

Triangle example using GLFW #1258

Conversation

Haggus commented May 8, 2017

kvark commented May 8, 2017

Haggus commented May 8, 2017

kvark commented May 9, 2017

Haggus commented May 9, 2017

Haggus commented May 10, 2017

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haggus commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented May 10, 2017

Haggus commented May 10, 2017

kvark commented May 10, 2017

kvark commented May 10, 2017

Haggus commented May 10, 2017

kvark commented May 10, 2017