V2.0 core dev #1995

rpicolet · 2017-10-09T12:45:11Z

WIP for render control architecture and logging/debugging utiliies

jwoolston · 2017-10-10T19:15:26Z

core/src/main/java/c/org/rajawali3d/control/RenderSurfaceView.java

+     *
+     * @param runnable
+     */
+    void queueEvent(Runnable runnable);


Can you please elaborate on the reasoning for both queueEvent and post?

queueEvent() posts to the render thread, post() to the main/UI thread, both are already present in the existing code, just including in the interface so CoreControl doesn't have to worry about which surface view impl is in use

I would prefer method names which reflected that.

[Not sure if my edit was before or after your comment, so moving forward I will stop editing except for spelling and such, not semantics or content]

Since the methods are already present in both GlesSurfaceView and GlesSurfaceTextureView, I just used them as-is so those classes did not have to be changed or introduce another delegation, but it makes sense looking forward to more surface view impls e.g. for Vulkan. I will change to queueToRenderThread() and queueToMainThread(). Good call.

Sounds good. Also, with our rather loose discussion about possibly extracting the render thread class, we basically will be keeping our own version of both TextureView and SurfaceView, so we can ultimately call them whatever we want.

jwoolston · 2017-10-10T19:20:15Z

core/src/main/java/c/org/rajawali3d/object/renderers/ObjectRendererImpl.java

-        if (isDoubleSided && !lastUsed.isDoubleSided()) {
-            // We are double sided, the last renderer is not - disable culling
-            GLES20.glDisable(GLES20.GL_CULL_FACE);
+        if (isDoubleSided) {


Why was this expanded into more verbose logic?

If isDoubleSided is true, but the lastUsed was also, then the else clause would be used...

But in that situation, there is nothing to do regarding double sided, which is why the clause was omitted originally.

The original else clause assumed not double sided and changed GL, so if the condition fails because both the current and last used are doubleSided, then GL will be configured for non-double-sided...

Should have said "The top-level else clause assumes not double sided and changesGL..."

jwoolston · 2017-10-10T19:22:32Z

core/src/main/java/c/org/rajawali3d/object/renderers/ObjectRendererImpl.java

-        } else if (lastUsed.isBlended()) {
-            // We are not blended, the last renderer is - disable
-            GLES20.glDisable(GLES20.GL_BLEND);
+        if (isBlended) {


Why was this expanded into more verbose logic?

If isBlended is true and lastUsed was also, then the else clause will be used.

Same as above

Same for me too

jwoolston · 2017-10-10T19:29:36Z

core/src/main/java/c/org/rajawali3d/control/RenderSurfaceView.java

+ *
+ * @author Randy Picolet
+ */
+public interface RenderSurfaceView extends SurfaceView {


Why not just fuse this class with SurfaceView?

There is actually no reason it should extend SurfaceView, which is the public client API. This is an internal API that defines what CoreControl needs from a surface view implementation. I will remove the extends clause.

I'm adding a PublicApi annotation to help with this since we need to clearly mark API anyway for everyone's good...

I would rather annote private APIs. Generally I want to try and protect these apis as much as possible with visibility, but in cases where that is not possible, we can annotate accordingly. Our public API is likely to be much larger than the truly private API, so its easier to maintain the private listing.

Using (I assume public) visibility consistently seems problematic, as there are numerous places where the engine needs to use public across its own packages, and numerous places where the client may need to exploit package/protected visibility.

But I'm ok with flagging internal types & methods versus external APIs, as long as we can tag a whole type rather than every method. Then, if we reliably maintain separate internal/external interfaces (i.e. don't allow individual interface methods to be tagged), and clearly specify that overridden methods inherit their API-ness (so we don't have to maintain tags all the way down the implementation/override tree), then we only have to also annotate internal methods that are not implementing an internal interface but are included in an non-internal class...

For example, we could just tag CoreControl as internal at the class level, and methods in CoreControl that implement e.g. the external RenderControl interface would not need to be tagged either. The tags are just documentation, so as long as the rules for their interpretation are also documented, then when a support issue comes in we can just say "sorry that's internal use only" and point them to the appropriate documentation.

Not sure about how to tag callbacks. They are technically internal, even though the client has to implement them... Maybe they just don't get tagged period, since all of that is obvious.

Not sure I fully understand what you are saying. Skip the annotation for now, we can decide on it at a future point.

OK for now. I will just have to read my own comments (instead of my own tags) to avoid mixing internal/external interfaces as I did here...

jwoolston · 2017-10-10T20:34:13Z

core/src/main/java/c/org/rajawali3d/control/gles/GlesRenderControl.java

+ * @author Randy Picolet
+ */
+
+public interface GlesRenderControl extends RenderControl {


I know that this additional interface level is specifically to handle Vulkan vs GLES, but you should document that so it is clear.

Forgot this existed. I'm not actually sure its needed, and I'm really sure it should not extend RenderControl...

If it doesn't serve a purpose I'm fine with nuking it. One less interface to track

jwoolston · 2017-10-10T20:42:32Z

core/src/main/java/c/org/rajawali3d/surface/gles/BaseGlesSurfaceRenderer.java


-    GLESRenderer(Context context, RenderSurfaceView renderSurfaceView,
-                 RenderControlClient renderControlClient, double initialFrameRate) {
+abstract class BaseGlesSurfaceRenderer extends CoreControl implements GlesRenderControl {


CoreControl implements RenderControl while GlesRenderControl extends it. We need to avoid this cycle.

Agreed, its wrong will fix.

jwoolston · 2017-10-10T20:43:46Z

core/src/main/java/c/org/rajawali3d/surface/gles/BaseGlesSurfaceRenderer.java

        super(context, renderSurfaceView, renderControlClient, initialFrameRate);
    }

    /**
-     * Callback to notify that the EGL context has been acquired and a valid GL thread with EGL surface now exists.
+     *
     */
    protected void onRenderContextAcquired() {


Don't omit @Override annotations.

Never on purpose, always flag em for me.

Although in this case its just the shared implementation, not overriding anything... Will document

What do you mean by shared implementation?

This method is used by both GL surface view renderers to get the context version number before forwarding up to CoreControl.onRenderContextAcquired(). Didn't want to duplicate the code in both places is all.

The problem is it implements an interface which defines a function of that name. Intentional or not, you overrode it.

The interface method is public and has context type/version parameters, this one is protected and has none. Different signatures. This one wraps/calls the interface method on behalf of both surface views, since they both figure out the version the same way; it does not override or extend the interface method.

Then I would argue one of them needs a different name.

That's legit, renamed it to notifyRenderContextAcquired.

jwoolston · 2017-10-10T20:48:27Z

core/src/main/java/c/org/rajawali3d/surface/gles/BaseGlesSurfaceRenderer.java

        super(context, renderSurfaceView, renderControlClient, initialFrameRate);
    }

    /**
-     * Callback to notify that the EGL context has been acquired and a valid GL thread with EGL surface now exists.
+     *
     */
    protected void onRenderContextAcquired() {


I know this is from old code, but the call to Capabilities.getInstance() here is redundant as we have established it in the configure() method of the surfaces.

I'm missing where else the Capabilities singleton gets created in the startup sequence... OTOH, I'm not sure what is gained by instantiating it if it is never used, so maybe this call to getInstance() should just be deleted?

My point was to remove it. It used to be used for populating the capabilities once a render context existed but I have changed it to no longer require that.

jwoolston · 2017-10-10T20:51:07Z

core/src/main/java/c/org/rajawali3d/control/BaseControl.java

+     * @param deltaTime
+     */
+    @RenderThread
+    protected abstract void onFrameStart(@FloatRange(from = 0.0) final double deltaTime);


I would like to retain the total elapsed time as well.

I think we had a discussion about this in the past, when talking about animations; the elapsed time is always available from RenderStatus.getFramesElapsedTime() when needed. I don't think we use/need it internally.

…effort Cleaned up surface, control, scene, and sceneview packages, reduced spaghetti, consolidated/renamed many types Added SurfaceConfiguration and Choreographer support

jwoolston · 2017-10-23T19:34:25Z

core/src/main/java/c/org/rajawali3d/annotations/InternalUseOnly.java

+ *  Flag a type or method for internal/engine use only, and specifically not supported for use by engine clients as
+ *  part of its external API
+ *
+ *  Flagging a method applies to all of its implementations/extensions/overrides; flagging a type implies flagging


What does flagging a type implies flagging all methods in that type mean?

It means tagging at the class or interface level means all the methods in that type are also internal only, so we don't have to tag every method in an internal-only class or interface. At your suggestion, I haven't used this anywhere. Instead/for now, I started using "Internal" in the actual name, which certainly makes the point more visible.

jwoolston · 2017-10-23T19:36:23Z

core/src/main/java/c/org/rajawali3d/annotations/RequiresRenderTask.java

+@Documented
+@Retention(RetentionPolicy.CLASS)
+@Target({ElementType.METHOD})
+public @interface RequiresRenderTask {


Off the cuff, I am concerned this may be confusing without much aid. I will reserve final judgment until I get through the entirety of this PR. In the meantime, this comment will serve as a reminder to myself to re-evaluate.

The point is to identify methods that should be called only in a render transaction.

In my next update, I've renamed the @RenderTask annotation to @RenderTransaction, and this one to @RequiresRenderTransaction, as I co-opted the RenderTask class name from its old use in 1.x for start-of-frame Renderer-queued tasks to the new render-thread-queued tasks that run completely outside of draw frame events. So in general, @RenderTransaction flags a client method that runs on the render thread but outside frame updates., so e.g. callbacks to clients running sequentially under a context creation, surface size change, or loss of context event control flow are @RenderTransactions, in addition to the new RenderTask that allows apps to do whatever they need to when they need to.

jwoolston · 2017-10-24T20:56:21Z

core/src/main/java/c/org/rajawali3d/control/RenderContextType.java

+ *
+ * @author Randy Picolet
+ */
+public enum RenderContextType {


Can we make this an android @IntDef annotation instead?

Makes sense, sure.

jwoolston · 2017-10-24T20:58:52Z

core/src/main/java/c/org/rajawali3d/control/RenderControl.java

+     *
+     * @param runnable
+     */
+    void queueToMainThread(Runnable runnable);


runnable should be annotated as @NonNull

Yep will do

jwoolston · 2017-10-24T20:58:59Z

core/src/main/java/c/org/rajawali3d/control/RenderControl.java

+     *
+     * @param runnable
+     */
+    void queueToRenderThread(Runnable runnable);


runnable should be annotated as @NonNull

Yep will do

jwoolston · 2017-10-24T21:03:06Z

core/src/main/java/c/org/rajawali3d/control/RenderControl.java

+
+    /**
+     * Removes a {@link Scene} from the list of Scene frame delegates. The {@link Scene} will be notified via
+     * {@link Scene#destroy()}, and should take any needed actions.


I know I'm responsible for this but I think we should consider changing this contract to be Scene#removed() instead of implying that removal equals destruction.

I'm not sure the callback is even necessary. I'm leaning towards adding/removing being internal operations that do the GL resource management under the covers, and letting the client do whatever they want before or after adding or removing. In other words, addToRenderControl() means allocate/configure whatever GL resources are necessary internally to start using the Scene at the next frame draw, and removeFromRenderControl() means release whatever resources a Scene is currently using because it will not longer participate in frame draws. That's why onAddToRenderControl() is part of the internal interface, the client shouldn't have to do anything

Ok, let's see what this fleshes out to.

Yeah, it's taken me a while to disentangle the old single-scene model lifecycle semantics from my brain, but I think its about there.

Of course, I've said similar things before...

jwoolston · 2017-10-24T21:06:19Z

core/src/main/java/c/org/rajawali3d/control/RenderControl.java

+
+    /**
+     * Removes a {@link SceneView} from the list of SceneView frame delegates. The {@link SceneView} will be notified
+     * via {@link SceneView#destroy()}, and should take any needed actions.


Same contract issue as with addScene()

And same response...

jwoolston · 2017-10-24T21:12:13Z

core/src/main/java/c/org/rajawali3d/scene/SceneModifier.java

- *
- * @author Jared Woolston (Jared.Woolston@gmail.com)
- */
-public interface SceneModifier {


Can you help me track what this turned into?

Nothing. Or rather, whatever the client would have wanted to do under this should be done in a RenderTask, along with any changes to its dependent SceneViews or to other Scenes/Sceneviews as needed by their app's logic. All part of getting away from fine-grained updates and providing atomic transactions of any granularity

rpicolet · 2017-10-27T04:47:33Z

Can’t find this comment on GitHub, replying via email I removed the @OverRide from FlatTree.intersect() because I removed that method from the SceneInternal interface and there was nothing left to override. i removed the method from the interface because I added the visibleObjectIntersection method intended to pre-filter NodeParents and invisible/hidden Object3Ds from the list handed to a SceneView and which may be iterated mutiple times and to simplify the rendering logic a bit.

…

On Oct 24, 2017, at 2:14 PM, Jared Woolston ***@***.***> wrote: @jwoolston commented on this pull request.here In core/src/main/java/c/org/rajawali3d/scene/graph/FlatTree.java <#1995 (comment)>: > @@ -77,7 +77,6 @@ public void ***@***.*** SceneNode added) { @RequiresReadLock @nonnull - @OverRide Why was this annotation removed? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1995 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlpmnEoMrJyHvsiiqXdmRdUvFR-J-DHks5svlMigaJpZM4Pybct>.

rpicolet and others added 6 commits June 28, 2017 14:12

Renaming before modifying to force Git to recognize the rename

d10e117

merging upstream v2.0-development

c7312f5

moving files to core from rajawali

ce55a31

Merging upstream v2.0-development updates

eeaffc0

WIP for render control architecture and logging/debug utilities

675ebe7

WIP for render control architecture and logging/debug utilities

3a16dab

jwoolston changed the base branch from master to v2.0-development October 10, 2017 02:29

jwoolston reviewed Oct 10, 2017

View reviewed changes

Rajawali deleted a comment from rpicolet Oct 10, 2017

jwoolston reviewed Oct 10, 2017

View reviewed changes

WIP for render control architecture and logging/debug utilities, 2nd …

db28fd5

…effort Cleaned up surface, control, scene, and sceneview packages, reduced spaghetti, consolidated/renamed many types Added SurfaceConfiguration and Choreographer support

jwoolston reviewed Oct 23, 2017

View reviewed changes

jwoolston reviewed Oct 24, 2017

View reviewed changes

V2.0 core dev #1995

Are you sure you want to change the base?

V2.0 core dev #1995

Conversation

rpicolet commented Oct 9, 2017

Choose a reason for hiding this comment

rpicolet Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpicolet Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoolston Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpicolet commented Oct 27, 2017 via email

rpicolet Oct 10, 2017 •

edited

rpicolet Oct 10, 2017 •

edited

jwoolston Oct 24, 2017 •

edited