Hi Cedric, Thanks for the review and the comments. I'm glad to hear that it worked =)
On Wed, Mar 16, 2011 at 11:35 PM, Cedric BAIL <cedric.b...@free.fr> wrote: > Hi, > > On Wed, Mar 16, 2011 at 8:26 AM, Sung W. Park <sung...@gmail.com> wrote: > > Based on the discussions and the comments that I've been getting, I'm > > attaching > > patches for the community to review and test out the code. The patches > > implement > > the code for the gl_x11 backend. Other backends can be taken care of > later > > once this has been reviewed. > > > > I apologize if this patch seems a bit big. I felt that we've had enough > > discussions > > on this topic already in increments and now we're at a point where we can > > see this > > prototype. > > I don't think them to big, they are really easy to understand and > follow. I really like that ! > > > Here are the description of the patches: > > (by the way, apply the patches by giving -p0 from evas directory) > > > > 1. Evas_Engine_GL_Context.patch > > - Basically, this patch replaces all the instances of Evas_GL_Context > used > > by the gl engines to Evas_Engine_GL_Context. This was changed because > > Evas_GL_Context is exposed to the users by Evas_GL.h and this was > ok'ed > > in the previous discussion. > > Ok, just reading it. Why do you need to add : > +#if defined (GLES_VARIETY_S3C6410) || defined (GLES_VARIETY_SGX) > +#include <EGL/egl.h> > +#else > +#include <GL/glx.h> > +#endif > This part of the code should not be there! It was from the previous hack that I did to expose the Evas' GL context. It was a foolish was but it did help me get my feet wet. I thought I deleted it but it managed to hide in there somehow. You can just delete it and it works fine. > > in src/modules/engines/gl_x11/Evas_Engine_GL_X11.h . What is the > purpose of this patch ? And why do you use evas private configure > define in a public header ? > > By the way the rest of the patch is ok to go in as it is. > > > 2. evas_gl.patch > > - This patch includes the image_object_native_surface_set() patch > > since it's required to run evas_gl and this patch hasn't been > reflected > > in the svn yet. > > - And, it has the rest of the code that allows evas_gl to run for gl_x11 > > engine. > > It seems you are currently destroying a surface using evas context > instead of the right context, because it could be destroyed. Maybe I > am wrong, but couldn't you implement a refcount attached to the user > context to prevent destruction as long as some surface are attached to > it. The rest of the patch looks good to me. Now we should wait for the > software version :-) Hum, maybe it would be good Vincent if you could > review it to, so we are sure nothing goes wrong when you try to port > this feature on Windows (I don't see anything obvious, but more eyes > is always better). > > Essentially, Evas_GL_Surface is implemented using an FBO and a texture and the texture can be seen both by the Evas' GL context and then user created context. the reason why I'm using Evas' context in the surface_destroy code instead of using the user context is because the user context can be destroyed first. I could potentially keep a ref count on the context and destroy it together but I didn't see a point in doing that here. it would work just fine using Evas' GL context. that's why i make sure that i unbind the context once i use it. maybe there is an angle that i'm not seeing here though. i'm definitely open to suggestions. =) > > 3. Evas_GL.h > > - Instead of putting it as part of the patch, I've included them > > separately so it > > can be viewed easily. > > - This files goes in > > src/lib/ > > > 4. evas_gl.c > > - Instead of putting it as part of the patch, I've included them > > separately so it > > can be viewed easily. > > - This files goes in > > src/lib/canvas/ > > Maybe in evas_gl_new you should check the validity of the Evas object. > And before calling any evas_gl->evas->engine.func, you should check if > they are correctly set. That's for the obvious, don't see much more > than that. > > Good point! I'll add that =) > > 5. examples/ > > - I've included 3 sample programs + Makefile > > a. evasglsample1.c (a simple triangle using GL) > > b. evasglgears.c (famous gears using Evas_GL) > > c. evasglessample1.c (a simple triangle using GLES) > > - You can simply do a make in the directory and it'll compile the > > evaglsampl1 > > and evasglgears by default. > > Hehe, looks nice :-) Examples are easy to understand and make this API > sounds good. > > > I just tested with a fresh co from SVN and have applied the patches. I > hope > > it works. > > I confirm it run well on my computer :-) > > > thanks in advance for your comments! > > That's good ! Could have been my only comment ;-) > -- > Cedric BAIL > Let me know if you guys have other comments/suggestions. cheers, Sung ------------------------------------------------------------------------------ Colocation vs. Managed Hosting A question and answer guide to determining the best fit for your organization - today and in the future. http://p.sf.net/sfu/internap-sfd2d _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
