jogl
jogl copied to clipboard
sgothel
Cleanup and OpenGL 3 support for TextRenderer
Currently all the updates are in RasterTextRenderer, but will just need to do a rename to TextRenderer next. Figured it might be easier to review that way, but also really wanted to get this started today so you could look at it. I'll try to add some more comments about it tomorrow.
I will go through your pull request and focus on feature duplication and namespace. The whole set of pull requests looks great, kudos - much better than the previous one.
- ShaderLoader: Please use the already existing ShaderUtil, maybe ShaderCode/Program .. etc. It utilizes IOUtil from GlueGen and allows resource loading even on mobile. We constantly enhance it .. maybe you have ideas for it as well ..
- AWT Testing Utilities sidelines the junit folder and is a unique folder, ok. However, all code there relates to your 'special' testing environment hence I would like to see it in it's own subfolder, choose your magic subfolder/project name. Another one: I lack to see a use-case for all these testing classes in this pull request. W/o usage .. I don't like to take it .. it's like dead code.
- Glyph* and your Text* impl. I am sure it's great - I still have to check it :) One little sad thing here is it still uses AWT, which does not allow mobile platforms to benefit from your work. You might have seen, I have added TTF support in jogamp.graph.font.typecast.** (importet the typecast project and did a bit cleanup) What do you think about using typecast as the Glyph source instead of AWT ?
Sorry, I don't mean to procrastinate things here .. will keep an eye on it and I hope I can try your code in 'a bit'.
The more your work is 'isolated' in it's own namespace denoting it's own sub-space, the earlier we can merge. Later on - as you suggested, we may merge the code into the top-level namespaces. Hopefully w/o AWT :)
I will go through your pull request and focus on feature duplication and namespace. The whole set of pull requests looks great, kudos - much better than the previous one.
Thanks!
ShaderLoader: Please use the already existing ShaderUtil, maybe ShaderCode/Program .. etc. It utilizes IOUtil from GlueGen and allows resource loading even on mobile. We constantly enhance it .. maybe you have ideas for it as well ..
I'll try to use them. At the time I was originally writing this it was easier to just load them myself since I only needed basic functionality, but I kept ShaderLoader package-protected with the goal of being able to replace it eventually.
AWT Testing Utilities sidelines the junit folder and is a unique folder, ok. However, all code there relates to your 'special' testing environment hence I would like to see it in it's own subfolder, choose your magic subfolder/project name. Another one: I lack to see a use-case for all these testing classes in this pull request. W/o usage .. I don't like to take it .. it's like dead code.
The testing utilites I added are used by GlyphRendererTest, QuadPipelineTest, and TextRendererTest in later commits. I just staged them separately to make it more digestible. I wasn't intending for the testing utilities to be public, they just resulted from realizing I was going to have a lot of duplicate code otherwise. I put everything in the awt package because GlyphRenderer and QuadPipeline are package-protected, and I wouldn't be able to test them separately otherwise.
One little sad thing here is it still uses AWT, which does not allow mobile platforms to benefit from your work. You might have seen, I have added TTF support in jogamp.graph.font.typecast.** (importet the typecast project and did a bit cleanup) What do you think about using typecast as the Glyph source instead of AWT ?
I think everything is broken out pretty well now, so that the changes that would need to be made would be relatively isolated, compared to what was there before at least.
However, having talked to Jason it sounds like we probably won't be able to justify spending much money on that. In general I guess we were just intending to keep TextRenderer usable as it was, except with OpenGL 3/4 support.
Maybe if we could get the conversion a little more concrete though... Is there a way with the typecast project to rasterize the font into a BufferedImage?
Ok, finally cleaned up and pushed the commit where I renamed RasterTextRenderer to TextRenderer. I'll work on cleaning up the shader loading, but having done that I'm comfortable in saying this could probably be pulled as is for now considering not much has changed publicly. The only thing I added to its interface was setTransform, since we don't have OpenGL's internal transformation stack in the core profile, and a constructor taking a UnicodeBlock, which is sort of unnecessary but it does allow for some optimization/simplification.
On 04/23/2012 05:13 PM, Andy Brown wrote:
I will go through your pull request and focus on feature duplication and namespace. The whole set of pull requests looks great, kudos - much better than the previous one.
Thanks!
ShaderLoader: Please use the already existing ShaderUtil, maybe ShaderCode/Program .. etc. It utilizes IOUtil from GlueGen and allows resource loading even on mobile. We constantly enhance it .. maybe you have ideas for it as well ..
I'll try to use them. At the time I was originally writing this it was easier to just load them myself since I only needed basic functionality, but I kept ShaderLoader package-protected with the goal of being able to replace it eventually. Good, so we may earmark this for a later cleanup.
AWT Testing Utilities sidelines the junit folder and is a unique folder, ok. However, all code there relates to your 'special' testing environment hence I would like to see it in it's own subfolder, choose your magic subfolder/project name. Another one: I lack to see a use-case for all these testing classes in this pull request. W/o usage .. I don't like to take it .. it's like dead code.
The testing utilites I added are used by GlyphRendererTest, QuadPipelineTest, and TextRendererTest in later commits. I just staged them separately to make it more digestible. I wasn't intending for the testing utilities to be public, they just resulted from realizing I was going to have a lot of duplicate code otherwise.
Ok, so we can (later on) create a sub-package for it to emphasize their intention for testing, good.I put everything in the awt package because GlyphRenderer and QuadPipeline are package-protected, and I wouldn't be able to test them separately otherwise. The public / private separation in JOGL doesn't necessarily rely on the Java module right now, but we simply indicate this by the package namespace.
'com.jogamp.' is public, i.e. intended for public use, where 'jogamp.' is semi private, i.e. not intended for public use but holds implementations.
I personally use private/protected/package-protected as well as final qualifiers as well, but more intended to clarify the intention and visibility. The access right qualifiers can't be used properly for security anyways,
In the 'dawn' of a future AWT-less implementation we may prepare the namespaces (package names) accordingly, at least I would like to do that if you don't mind.
One little sad thing here is it still uses AWT, which does not allow mobile platforms to benefit from your work. You might have seen, I have added TTF support in jogamp.graph.font.typecast.** (importet the typecast project and did a bit cleanup) What do you think about using typecast as the Glyph source instead of AWT ?
I think everything is broken out pretty well now, so that the changes that would need to be made would be relatively isolated, compared to what was there before at least. Of course! I don't compare against the 'old' quality here :)
However, having talked to Jason it sounds like we probably won't be able to justify spending much money on that. In general I guess we were just intending to keep TextRenderer usable as it was, except with OpenGL 3/4 support. I see. Maybe I will go through such a task, since for my and colleagues we consider an AWT only impl. to be broken. Meaning we love to see things on mobile w/o the need for AWT. This also includes GLES2 ofc. But I don't know when I have time for this either, so I guess merging your work now seems to be a good time.
Maybe if we could get the conversion a little more concrete though... Is there a way with the typecast project to rasterize the font into a BufferedImage?
The latter is AWT as well IMHO. If you require the curves rasterized we would need an implementation for it - right, you cache the current AWT font rasterization and use textures ? Sorry for being no expert of the details here (yet).
Bottom line, as you said, you replace the previous font renderer with a better implementation and added functionality regarding the used GL profiles. This alone should let us finally merge.
Kudos!
~Sven
Ok, this doesn't exactly inspire confidence but I ran the the test for TextRenderer that was in there before and realized I introduced a NullPointerException when the user calls setUseVertexArrays before beginRendering. Working on fixing that now.
Ok, fixed the bug with setUseVertexArrays, and one with dispose, too. Cleaned up ShaderLoader a bit, where now we pass it a GL, validate the program, and at least most of the heavy functionality kicks to ShaderUtil.
Does this text renderer still support OpenGL 1.1? Sorry for my silly question.
Turns out not so silly... I was sure I saw somewhere that a few of the glPixelStore parameters were only in OpenGL 1.2, so I had a check in there for that, but I just went back to make sure and couldn't find that again. It looks like it was spelled out a little more explicitly in the 1.2 spec though so maybe that's why. Anyway, the short answer is yes (now) it should support 1.1 again. Thanks for asking!
It would be nice if this pull request was merged as some users are complaining about the inability of using the legacy text renderer with GL3.
If we get a clean merge request, as communicated post Siggraph 2012 .. and hence a bit of maintenance .. sure. But I haven't heard back from 'adbrown85' and his peers.
@sgothel I merged your master branch in -- I think this should be OK to pull again. Maybe afterwards we can work on moving the package-private utilities to the jogamp namespace, which would then clear the way to move the tests too?
Relevant output from git log -- TextRenderer.java:
1ec82447e464d5308442581f14d32f9775928454 Relocating javax.media.opengl.* -> com.jogamp.opengl.* (Part 1) 947bf9c45261013d81cc7199cb71c89b88b18fdf Findbugs: Use static fields where possible a41db57df54863566b0e286cd100bbbc5518eb7f Findbugs: Use NumberType.valueOf(..) instead of 'new NumberType(..)' 96d530e7127c89db9991080e6268c6e8430d0619 Findbugs.not-written.null: Fix referencing non-written fields (never written or due branching) 556d92b63555a085b25e32b1cd55afce24edd07a Code Clean-Up based on our Recommended Settings 21b84da775fae5806481ecc658a207bf603126d5 GLU: Make ProjectFloat/ProjectDouble final and deprecate GLU.destroy() method 7f7a23dd0ddf106e6f0c69fc2a05ff92ac56200e Bug 776 GLContext Sharing: Refine API for relaxed and lazy GLContext sharing; Fix GLContext memory contract (volatile) 6d841a02d084142e2f90333a420cd39af1188d80 Enable generics annotations on TextRenderer f1ae8ddb87c88a57dce4593f006881ef6a7f0932 Add missing Override annotations 5e9c02bce7b241a0bf95c8abca9a91cd25e51ed3 Remove all trailing whitespace 8ac3f344aded383ca9a3083a877af7bfdf6e1e48 Remedy for Bug 782: Issue Debug.initSingleton() or Debug.debug(..) before calling 'PropertyAccess.isPropertyDefined(propName, default)' through Debug class 20bf031db719f7baa4c6e74734fc999061e08fe2 FBObject / Offscreen Support - Part 1 ffb47c6610fc742926abc946fc14d12ee9c65a9a StringBuffer -> StringBuilder 7d7e7c901d8fe54af1230cbf10e568f1a8433cbe Adapt to gluegen Properties/Security commits
Since I dropped the "debug window" from TextRenderer I don't think 7f7a23dd0ddf106e6f0c69fc2a05ff92ac56200e, 8ac3f344aded383ca9a3083a877af7bfdf6e1e48, or 7d7e7c901d8fe54af1230cbf10e568f1a8433cbe apply anymore.
As you might have already noticed I took care of using GLExtensions (except for VERSION_1_1 which isn't there), but should probably check the constants are being accessed directly... Other than that I think the rest is style-related that should be OK, e.g., declaring variables final.
RectanglePacker was the only other conflict, but that was pretty straightforward to merge manually. Basically I had just added a constructor where you could specify the expansion factor.
@adbrown85 Great job, thank you very much. I'll spend some time during this weekend to study your changes. ShaderLoader is useless (to me).
Spent awhile cleaning it up over the weekend. Would like to do a bit more but ran out of time. Monday nights are usually pretty open for me though so I might be able to finish it up tonight.
I'd like to see this pull request in JOGL 2.3.2 or 2.4. I'll talk about it to @sgothel
Awesome! I have one outstanding change still, which was to use JUnit rules to run my tests on the Swing thread because they're a little wonky / messed up right now, but I guess that could be a separate pull request too. So far I was just testing it in a separate project so I'm not sure if it'll work with JOGL's version of JUnit, etc. though.
On Wed, Aug 26, 2015 at 10:50 AM, Julien Gouesse [email protected] wrote:
I'd like to see this pull request in JOGL 2.3.2 or 2.4. I'll talk about it to @sgothel https://github.com/sgothel
— Reply to this email directly or view it on GitHub https://github.com/sgothel/jogl/pull/47#issuecomment-135047110.
Is this change absolutely necessary? Yes, rather put it into a separate pull request. Please bump Sven on IRC, it would be nice that you talk about your changes to him. In my humble opinion, your code shouldn't be wasted, this is something that I would like to see in JOGL, really.
On 08/27/2015 03:50 PM, Andy Brown wrote:
Awesome! I have one outstanding change still, which was to use JUnit rules to run my tests on the Swing thread because they're a little wonky / messed up right now, but I guess that could be a separate pull request too. So far I was just testing it in a separate project so I'm not sure if it'll work with JOGL's version of JUnit, etc. though.
Unit tests within our framework would be more than welcome - of course!
Further, since Julien seems to jump in here, voting for a merge, I ask Julien: Can you please manually test this branch in regards to this work? Hint: Regressions.
If Julien gives me the green light (after manually validating no regression) and if your (Andy) unit tests arrive and pass - I gladly will merge!
@Julien: Since this is the first commit of Andy, we would also need to review for possible security issues as well. Usually we always should do that, of course. Hint: Any new or modified privileged code, e.g.:
- 'AccessController.doPrivilege'
- JNI code should raise concerns of course.
I know, previous code base had almost no unit tests, but a simple 'black box' one(s). Still - since you have unit tests, they would be more than appreciated!
Even though this list might seem a bit long, thank you Andy for catching up with your 2-3 year (?) long work here.
~Sven
I'm using this version of TextRenderer for GL3 on Mac OSX but I had to fix some bugs:
- the use of VAO was forced, I have made it working with setUseVertexArrays method of TextRenderer (classes GlyphRendererGL3, QuadPipelineGL30)
- the setColor method set wrong color, using Green component also for Blue (method doSetColor of GlyphRendererGL3)
- calling beginRendering for the first time with a not white (1, 1, 1, 1) color threw an exception, I have created a init() method to call before start rendering (maybe this method is not necessary, maybe there's a better solution)
Then it works perfectly.
@paolofuse Awesome!
Can you make a fork for me to pull in your changes? Or send me some patches from format-patch?
Also, can you explain why you don't want to or can't use a VAO with 3? You're not using a core profile I take it? It's been awhile and I don't do a lot of OpenGL anymore but I think the reason it's forcing it is I thought VAOs are required in core. If we do still allow it should we put a check in for core? Can't remember is JOGL GL3 supposed to always be core? I guess GL3bc or something like that extends GL3?
@paolofuse Is the init method something the user calls on TextRenderer? Or is it on one of the helper classes?
@adbrown85 GL3bc extends GL2 and GL3, it means "GL3 backward compatible. Therefore, you can get a GL3 instance that isn't core, use GLBase.isGL3core() to check that. Keep in mind that there is a default VAO created under the hood in JOGL.
Hi all, here you can find the fork from adbrown85 branch: https://github.com/fusedeveloper/jogl
Feel free to comments my solution. Thanks
@adbrown85 the init method must be call by the user before call beginRenderer.
@adbrown85 @gouessej what's the status on this?
I want to use this because I can't get TextRegionUtil to work properly.
@jedwards1211 Sorry for the delay. You should ask for help on the forum concerning TextRegionUtil. We'll try to integrate this stuff in JOGL 2.4.0 if we have enough time to fix and test it anew.