search

flyx / OpenGLAda

Thick Ada binding for OpenGL and GLFW
flyx.github.io/OpenGLAda/
MIT License
95 stars 13 forks source link

Initial commit of mesh-view example #94

Closed leo-brewin closed 5 years ago

leo-brewin commented 6 years ago

I've created a branch that contains just the addition of the mesh-view example. I'm not sure if I've followed correct Git workflow and I'm happy to take any advice (including starting from scratch :).

rogermc2 commented 6 years ago

Thanks to Google, I now have mesh-view branch and have been able to build and run meshview which runs impressively. I'm pretty sure that one of the first things that Felix will want is for your common to be merged with the existing common. I presume that he will want at least that done before he merges the PR. Before I can do anything with your code I need to find out how to synchronize our two branches.

rogermc2 commented 6 years ago

I've just changed all common files, except the initialize files, to use examples/common files and all works well. Of course these changes are only on my computer. Just looking at your Initialize, I should take Glfw.Input.Mouse.Cursor_Mode and Show_GL_Data out of my version as they should be left up to the user's main initialization program. Out of interest, is adding Class to the Main_Window declaration necessary for callbacks? So far, I've not used them. As its an improvement, I should probably chage my Initialize to include. Actually I should probably just replace my Initialize with yours.

rogermc2 commented 6 years ago

Just a small point; we are now including file names in error messages as an aid to users. E.g "An exception occurred in meshview.meshview."

rogermc2 commented 6 years ago

I'm wondering if your keyboard and mouse callbacks could be moved to common/controls as they appear to be of general use.

rogermc2 commented 6 years ago

I suspect libobj and libpng could also be moved to examples/common as, presumably, they are generally useful. Did you consider using streams (if appropriate)? Felix recommended streams to me when I was developing load_dds and I found them a great improvement over what I had originally. All those Get (line statements might be candidates?

rogermc2 commented 6 years ago

Felix doesn't like commented out code as it can be a distraction to users of the code. Commented out code should be removed.

flyx commented 6 years ago

@rogermc2 Actually, I am not that fond of exception messages including package / subprogram names. I just kept them in your PR because there were more important things to tackle. There's actually no need to include the subprogram's name in the exception message because locating an exception's origin is already a feature of GNAT. It can easily be used with the visual debugger in GPS and in addition to the exception source line, it provides a complete stack trace. I have no strong opinion on it, but this is definitely not a prerequisite to get a PR merged ;).

It may take a while for me to review this, since current focus is merging #87 .

rogermc2 commented 6 years ago

With regard to Initialize, I now recall that my original idea with respect to such overridable things as cursor was that I would initialize to a default setting and that the user could override them in their own application specific "setup" routine, which I think is what I've done in at least one example. My current thinking is that I'll remove Show_GL_Data from Initialize and, if agreed, add Class to the Main_Window declaration. Although these are only small changes I intend to create a separate branch update_intialize to implement them because of their potential impact on many examples. For example I would like to include Show_GL_Data in basic examples. Also, if Class is implemented, then all examples will need change. Does this seem OK?

leo-brewin commented 6 years ago

Greetings Felix and Roger,

I'm in no particular rush to make any changes at the moment -- I'm more than happy to wait for Felix to review the PR before I do anything. But I do agree with Roger that it's not sensible to have two separate "common" directories. At some point these should be consolidated into one (with the required changes made to all the other examples).

Cheers, Leo

On 4 October 2017 at 21:27, Roger notifications@github.com wrote:

With regard to Initialize, I now recall that my original idea with respect to such overridable things as cursor was that I would initialize to a default setting and that the user could override them in their own application specific "setup" routine, which I think is what I've done in at least one example. My current thinking is that I'll remove Show_GL_Data from Initialize and, if agreed, add Class to the Main_Window declaration. Although these are only small changes I intend to create a separate branch update_intialize to implement them because of their potential impact on many examples. For example I would like to include Show_GL_Data in basic examples. Also, if Class is implemented, then all examples will need change. Does this seem OK?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flyx/OpenGLAda/pull/94#issuecomment-334114200, or mute the thread https://github.com/notifications/unsubscribe-auth/ADxPtSrwZH8_Qj0AHm3Viqklp_mls6ASks5so115gaJpZM4Ps-HH .

flyx commented 6 years ago

So I finally can have a look at this. I won't read all the code, but there are some things I stumbled upon:

leo-brewin commented 6 years ago

I'm pretty well flat out with other things too so I'm happy with the delay in processing the pull request :)

flyx commented 6 years ago

I am very sorry, I have left this lying around for a looong time.

I just tried to build and execute your code. Right after running, a window pops up and closes again, then I get

raised PROGRAM_ERROR : EXCEPTION_ACCESS_VIOLATION

Any idea what might cause this? What operating systems have you tested this with?

leo-brewin commented 6 years ago

Hi Felix,

No problems. I too have had a lot on my plate (and sorry about my slow repose to you :).

I'm on a Mac running macOS High Sierra 10.13.3. The glfw and glew libs were installed using the "brew" package manager for macOS. The versions are: glfw-3.2.1 and glew-2.1.0. I have recompiled the OpenGLAda and mesh view codes and they both run fine on my mac. I don't know what the problem could be.

The only libs that I use that aren't used anywhere else in OpenGLAda are the png and zlib libraries. The libPNG is used in only routine, libobj.adb. The declare block in lines 252-274 serve to set the colours for each vertex in the mesh. That block could be changed to set the colours to a single shade of grey (just for testing). If the code then runs then it's clear that the access violation has something to do with the png/zlib libs. I know I'm plucking at straws. But given that the rest of OpenGLAda runs fine I suspect the problem must be in the interaction between OpenGLAda and the png/zlib's.

Cheers, Leo