Re: [E-devel] [Review] [Patch] Evas - OpenGL and Evas (Part 4)

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Hi Daniel,

Can you elaborate on what you mean by making this as a patch?

I've sent two patches for the files that need to be updated, one of which
has been applied
already.  The other one has been reviewed a little but hasn't been applied
yet.

As for the other files, they need to be freshly added to the svn and since i
don't have
commit access, i've just attached them separately.

Let me know what you mean by it.

cheers,
Sung

On Thu, Mar 24, 2011 at 12:32 AM, Daniel Juyung Seo <seo...@gm...>wrote:

> Can you make this as a patch?
>
> Thanks.
> Daniel Juyung Seo (SeoZ)
>
> On Thu, Mar 17, 2011 at 11:34 PM, Sung W. Park <su...@gm...> wrote:
> > Hi again,
> >
> > On Thu, Mar 17, 2011 at 2:39 AM, Cedric BAIL <ced...@fr...>
> wrote:
> >
> >> Hi again,
> >>
> >> On Wed, Mar 16, 2011 at 5:02 PM, Sung W. Park <su...@gm...>
> 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 <ced...@fr...>
> >> wrote:
> >> >> On Wed, Mar 16, 2011 at 8:26 AM, Sung W. Park <su...@gm...>
> >> 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.
> >>
> >
> > Great!  That email address is fine.  wow, thanks.
> >
> >
> >>
> >> >> 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.
> >>
> >
> > It would be cleaner.  Maybe I'll revisit it at a later time.  I'm taking
> > tomorrow
> > off to get some rest finally =)
> >
> >
> >>
> >> >> > 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 ! :-)
> >>
> >
> > LOL.  yeah, it was only a little past midnight when I sent out that
> email.
> >
> > By the way, I have added
> >
> >   MAGIC_CHECK(e, Evas, MAGIC_EVAS);
> >   return NULL;
> >   MAGIC_CHECK_END();
> >
> > in evas_gl_new() as you've suggested.  It's probably not necessary but
> I'll
> > just attach the file anyway.
> >
> > cheers,
> > Sung
> >
> > --
> >> 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
> > enl...@li...
> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >
> >
>