Added GLFW specific functionality from GLWindow.jl #118

louisponet · 2017-12-28T22:34:54Z

moved functions from glfw3.jl to functions.jl
moved types from glfw3.jl to types.jl
moved globals from glfw3.jl to constants.jl
move gl_createcontext out of GLWindow.jl/screen.jl to a constructor for Window.
Added a lot of Window related functionality from GLWindow.jl/screen.jl,types.jl,core.jl to types.jl.
Added MonitorProperties from GLWindow.jl/types.jl to types.jl
Changed MonitorProperties so it doesn't depend on GeometryTypes: Vec hit in performance shouldn't matter imo.
Added general GLFW.jl related functionality from GLWindow/screen.jl,core.jl to extensions.jl

Tests work, also ran the tests for the pullrequest in GLWindow.jl

General Idea: Firstly, splitting pure GLFW.jl related functionality from GLWindow.jl will allow it to be less backend specific, and act as a interface to different OpenGL window/context providers. This is in light of hopefully seperating Windowing functionality in 'GLWindow' from GLAbstraction.jl and provide more of an API experience.
Secondly, I understand this makes the package no-longer a barebones wrap around the GLFW library, but was it really purely that to begin with, and would people not use the added functionality that only depends on GLFW if it is there?

Made it so MonitorProperties doesn't depend on GeometryTypes, I think for performance its ok. If something constructs it without using the constructor there might be some trouble in current implementations

jayschwa

Reorganizing files makes it hard to review the functional changes. Please do either one or the other in this PR.

For file reorganization, I think it makes more sense to group things by functionality (e.g. context, input, window, etc) rather than by kind (e.g. functions, types, constants). http://www.glfw.org/docs/latest/modules.html groups API names by functionality and can probably inform a reorg.

louisponet · 2017-12-28T23:28:12Z

I agree on functionality over "kind". Wouldn't this create a lot of very small files?

jayschwa · 2017-12-29T01:12:13Z

Thanks for dialing back the file reorg. It will make the other changes easier to digest.

SimonDanisch · 2017-12-29T12:29:09Z

src/glfw3.jl

+    props = MonitorProperties(GetPrimaryMonitor())
+    w,h = props.videomode.width, props.videomode.height
+    # Vec(Int(w),Int(h))
+    Vector([Int(w),Int(h)])


Just return (Int(w), Int(h))

This should keep previous functionality without depending on GeometryTypes, with the same performace as well

jayschwa

Removing the ceremony from changing window properties and including saner defaults seems good. Some of the other additions aren't obvious wins to me, so I requested removal. I'm open to having them added in future PRs where they can be debated.

If this gets merged, I'll probably still tweak things a bit before cutting a new release.

jayschwa · 2017-12-31T19:20:51Z

src/glfw3.jl

@@ -1,4 +1,4 @@
-#************************************************************************
+************************************************************************


Fix comment by re-adding #.

jayschwa · 2017-12-31T19:25:18Z

src/glfw3.jl

+
+function poll_glfw()
+    PollEvents()
+end


Remove this function. It is not adding value.

jayschwa · 2017-12-31T19:27:55Z

src/glfw3.jl

+    Integer: The number of the Monitor to Select
+    Bool: if true, primary monitor gets fullscreen, false no fullscren (default)
+    GLFW.Monitor: Fullscreens on the passed monitor
+"""


Remove this function. It appears to be unused.

jayschwa · 2017-12-31T19:28:34Z

src/glfw3.jl

+        KEY_UP    == b && return :up
+    end
+    return :nothing
+end


Remove this function. It appears to be unused.

jayschwa · 2017-12-31T19:33:30Z

src/glfw3.jl

+
+    MakeContextCurrent(window)
+
+    debugging && glDebugMessageCallbackARB(_openglerrorcallback, C_NULL)


glDebugMessageCallbackARB appears to be undefined. This line might need to be removed.

jayschwa · 2017-12-31T19:34:10Z

src/glfw3.jl

+    window.handle == C_NULL && return
+    SwapBuffers(window)
+    return
+end


Remove this function. It's not adding enough value in my opinion.

jayschwa · 2017-12-31T19:41:25Z

src/glfw3.jl

+        (Symbol(last(split(string(f),"."))), f(window))
+    end
+    Dict{Symbol, Any}(tmp)
+end


Remove this function (for now). I think it's a little too magical.

jayschwa · 2017-12-31T19:46:12Z

src/glfw3.jl

+
+function Base.resize!(x::Window, w::Integer, h::Integer)
+    SetWindowSize(x, w, h)
+end


Remove this function (for now). The documented description of resize! is for collections, so using it here seems a bit off. The alternative (calling SetWindowSize) isn't any longer.

jayschwa · 2017-12-31T19:49:56Z

src/glfw3.jl

+function Base.isopen(window::Window)
+    window.handle == C_NULL && return false
+    !WindowShouldClose(window)
+end


Remove this function (for now). "is open" and "should not close" are two different concepts in my opinion, and the alternative can still be done in one line.

I agree on all of the comments. Some of the functions aren't used/belong somewhere else like you correctly assessed! I will make sure to pay more attention to these things in the future.

louisponet · 2018-01-02T18:01:43Z

I'm not against any tweaking on your part I'm sure you know better what to do with this package than I do. If you want I can also delete all the annotations I made, like where blocks of code originated from.

jayschwa

A few changes requested. The annotations don't need to be removed. I like seeing where they came from. Once tests are green, I'll merge and take it from there.

jayschwa · 2018-01-03T03:08:11Z

.gitignore

@@ -1,2 +1,3 @@
 deps/*
 !deps/build.jl
+


Remove this new line.

jayschwa · 2018-01-03T03:08:26Z

src/glfw3.jl

@@ -269,16 +269,103 @@ end
 Base.show(io::IO, m::Monitor) = write(io, "Monitor($(m.handle == C_NULL ? m.handle : GetMonitorName(m)))")

 const WindowHandle = Ptr{Void}
+


Remove this new line.

jayschwa · 2018-01-03T03:09:47Z

src/glfw3.jl

@@ -298,6 +385,39 @@ immutable GLFWError <: Exception
 end
 Base.showerror(io::IO, e::GLFWError) = print(io, "GLFWError ($(e.code)): ", e.description)

+#Came from GLWindow.jl/types.jl
+struct MonitorProperties 


Change struct to immutable for Julia 0.5 compatibility.

This request was made irrelevant by #121. However, I couldn't get GitHub to resolve merges through it's UI, so I did it manually and push it to #122.

jayschwa · 2018-01-03T03:14:45Z

Also, not sure if you care, but GitHub is setting your name to "Unknown" as the author. I believe that can be changed under GitHub settings.

jayschwa · 2018-01-05T03:12:24Z

Merged via #122. Thanks for the contribution!

louisponet · 2018-01-05T10:44:40Z

Very Good, thanks to you too!

Added GLFW specific functionality from GLWindow.jl

f3a3b2c

Made it so MonitorProperties doesn't depend on GeometryTypes, I think for performance its ok. If something constructs it without using the constructor there might be some trouble in current implementations

louisponet mentioned this pull request Dec 28, 2017

moved some functionality to GLFW.jl JuliaGL/GLWindow.jl#55

Open

jayschwa self-requested a review December 28, 2017 22:44

jayschwa requested changes Dec 28, 2017

View reviewed changes

Changed to put everything in glfw3.jl

d9d79fa

SimonDanisch reviewed Dec 29, 2017

View reviewed changes

Changed Vectors to NTuples to mimic previous Vec behaviour

be8b95a

This should keep previous functionality without depending on GeometryTypes, with the same performace as well

jayschwa requested changes Dec 31, 2017

View reviewed changes

Went through requested changes.

60ccda5

I agree on all of the comments. Some of the functions aren't used/belong somewhere else like you correctly assessed! I will make sure to pay more attention to these things in the future.

jayschwa requested changes Jan 3, 2018

View reviewed changes

jayschwa added 2 commits January 4, 2018 21:55

Merge branch 'master' into pull-request/f3a3b2ce

6d9e01f

Merge branch 'master' into pull-request/f3a3b2ce

65c04f8

jayschwa closed this Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added GLFW specific functionality from GLWindow.jl #118

Added GLFW specific functionality from GLWindow.jl #118

louisponet commented Dec 28, 2017 •

edited

Loading

jayschwa left a comment

louisponet commented Dec 28, 2017 •

edited

Loading

jayschwa commented Dec 29, 2017

SimonDanisch Dec 29, 2017

jayschwa left a comment

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

jayschwa Dec 31, 2017

louisponet commented Jan 2, 2018

jayschwa left a comment

jayschwa Jan 3, 2018

jayschwa Jan 3, 2018

jayschwa Jan 3, 2018

jayschwa Jan 5, 2018

jayschwa commented Jan 3, 2018

jayschwa commented Jan 5, 2018

louisponet commented Jan 5, 2018

		@@ -1,4 +1,4 @@
		#************************************************************************
		************************************************************************


		MakeContextCurrent(window)

		debugging && glDebugMessageCallbackARB(_openglerrorcallback, C_NULL)

		@@ -269,16 +269,103 @@ end
		Base.show(io::IO, m::Monitor) = write(io, "Monitor($(m.handle == C_NULL ? m.handle : GetMonitorName(m)))")

		const WindowHandle = Ptr{Void}

Added GLFW specific functionality from GLWindow.jl #118

Added GLFW specific functionality from GLWindow.jl #118

Conversation

louisponet commented Dec 28, 2017 • edited Loading

jayschwa left a comment

Choose a reason for hiding this comment

louisponet commented Dec 28, 2017 • edited Loading

jayschwa commented Dec 29, 2017

Choose a reason for hiding this comment

jayschwa 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

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

louisponet commented Jan 2, 2018

jayschwa 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

jayschwa commented Jan 3, 2018

jayschwa commented Jan 5, 2018

louisponet commented Jan 5, 2018

louisponet commented Dec 28, 2017 •

edited

Loading

louisponet commented Dec 28, 2017 •

edited

Loading