Hi again, On Wed, Mar 16, 2011 at 5:02 PM, Sung W. Park <sung...@gmail.com> wrote: > 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: >> 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.
So that one is in svn. I also added you in the AUTHORS file, if you prefer another mail address, please let me know. >> 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. =) Well, it works, it's just it would be cleaner imho, but as you say not that much necessary. Not a blocker in my opinion. >> > 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 ;-) > > Let me know if you guys have other comments/suggestions. Yeah, stop sleeping and review that patch ! :-) -- Cedric BAIL ------------------------------------------------------------------------------ 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
