Added GLFW specific functionality from GLWindow.jl by louisponet · Pull Request #118 · JuliaGL/GLFW.jl

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

+    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

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


Fix comment by re-adding #.

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

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


Remove this function. It is not adding value.

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

+    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

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


Remove this function. It appears to be unused.

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

+
+    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

+    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

+        (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

+
+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

+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

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


Remove this new line.

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

 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

 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

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


		MakeContextCurrent(window)

		debugging && glDebugMessageCallbackARB(_openglerrorcallback, C_NULL)

		Base.show(io::IO, m::Monitor) = write(io, "Monitor($(m.handle == C_NULL ? m.handle : GetMonitorName(m)))")

		const WindowHandle = Ptr{Void}

Uh oh!

Conversation

louisponet commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayschwa left a comment

Choose a reason for hiding this comment

Uh oh!

louisponet commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayschwa commented Dec 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayschwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisponet commented Jan 2, 2018

Uh oh!

jayschwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayschwa commented Jan 3, 2018

Uh oh!

jayschwa commented Jan 5, 2018

Uh oh!

louisponet commented Jan 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

louisponet commented Dec 28, 2017 •

edited

Loading

louisponet commented Dec 28, 2017 •

edited

Loading