vaiorabbit / ruby-opengl

Yet another OpenGL wrapper for Ruby (and wrapper code generator).
Other
86 stars 11 forks source link

about the load_dll method #7

Closed cedlemo closed 9 years ago

cedlemo commented 9 years ago

I am doing some tests on linux and I was about to modify the script sample/utils/setup_dll in order to make it work on a basic linux installation with those informations:

libname.so in /usr/lib

Don't you think that it would be better to directly set this default in the load_dll def :

grep load_dll lib/*                                                                                                                                          
    lib/glfw.rb:  def self.load_dll(lib = nil, path = nil)
   lib/glfw30.rb:  def self.load_dll(lib = nil, path = nil)
   lib/glu.rb:  def self.load_dll(lib = nil, path = nil)
   lib/glut.rb:  def self.load_dll(lib = nil, path = nil)
   lib/opengl_common.rb:  def self.load_dll(lib = nil, path = nil)

And let the user add parameters for custom/local libraries informations?

If it is ok for you I can send you the related pull request (each will be tested before created of course).

A last question : as a Linux user the name of the method doesn't make any sense, it must be the same for OSX users because the dll are window shared lib.

Could it be possible to change that name like something more general load or _loadlib. The use of alias or alias_method can help to with retro compatibility and deprecated message.

vaiorabbit commented 9 years ago

If you want to add Linux default lib/path information, please modify like:

    def self.load_dll(lib = nil, path = nil)
      if lib == nil && path == nil
        case OpenGL.get_platform
        when :OPENGL_PLATFORM_WINDOWS
          lib, path = 'GLFW3.dll', Dir.pwd
        when :OPENGL_PLATFORM_MACOSX
          lib, path = 'libglfw.dylib', Dir.pwd
        ########## ↓↓↓↓ ##########
        when :OPENGL_PLATFORM_LINUX
          lib, path = 'libglfw.so', '/usr/lib'
        ########## ↑↑↑↑ ##########
        else
          lib, path = 'libglfw.so', Dir.pwd
        end
      end

I think any platform-specific lib/path names shouldn't be used as default arguments. Normally, users are responsible for filling these arguments explicitly.

It's preferable for me to rename 'load_dll' to 'load_lib'. Thank you.

cedlemo commented 9 years ago

I think any platform-specific lib/path names shouldn't be used as default arguments.

So ok I just modify sample/utils/setup_dll but I can rename load_dll to load_lib

Do you want that I create a load_dll that call load_lib with a deprecated error message?

vaiorabbit commented 9 years ago

Do you want that I create a load_dll that call load_lib with a deprecated error message?

Yes. That's fine.

cedlemo commented 9 years ago

I have done all the related changes so I close this issue. Do you want that I update the:

Let me know

vaiorabbit commented 9 years ago

README about the load_lib method all the samples that use the deprecated loader

Yes, please do them.

Also, if you have time, do you mind updating the 'Tested Environment' section in README.md ?

Thank you.

cedlemo commented 9 years ago

Of course, I will do that