Renderer Task Queue

[jwoolston]

This was initially brought up in #543 but I thought it deserves a dedicated discussion.

I have had need of adding and removing objects from the scene at run time after it is initially created primarily for testing the scene graph stuff I'm working on but I believe I will need to do this in the app that I am currently working on as well. I am planning to work on a solution to this after I finish up the scene graph so I would like to start getting peoples thoughts on a good way of doing this. I know something to this effect could be accomplish by controlling if things are hidden or not but this seems cumbersome and not fully effective to me.

@ToxicBakery suggested a queue of some kind but from what I understand has concerns about the overall latent weight this would have. Following @MasDennis's suggestion in #112 someone can add object in onDrawFrame() but iterating them has some definite concurrency risks, which have been reduced with #546.

I will need something to be able to do this so I certainly don't mind doing it in a way you guys would be agreeable with. Anyone have some thoughts to share?

---

TODO List

• Create queue and queuing functions
• Add PostProcessingFilter support Not handling this at the moment
• Update RajawaliExamples
• Handle Add Task
  • Animation
  • BaseObject3D
  • Camera
  • Lights
  • Plugins
  • Texture
  • PostProcessingFilter
• Handle Remove Task
  • Animation
  • BaseObject3D
  • Camera
  • Lights
  • Plugins
  • Texture
  • PostProcessingFilter
• Handle Add All Task
  • Animation
  • BaseObject3D
  • Camera
  • Lights
  • Plugins
  • Texture
  • PostProcessingFilter
• Handle Remove All Task
  • Animation
  • BaseObject3D
  • Camera
  • Lights
  • Plugins
  • Texture
  • PostProcessingFilter
• Handle Replace Task
  • Animation
  • BaseObject3D
  • Camera
  • Lights
  • Plugins
  • Texture
  • PostProcessingFilter

[MasDennis]

Thanks for picking this up.

I'm not sure if there's an alternative to a queue. The problem that occurs when trying to create objects during a render cycle is that it will fail when trying to create shaders, textures, etc. The quick & dirty solution in #112 reflects this. Here we just wait for the rendering to finish and then we create our objects.

There might be an alternative but I'm not aware of it. I'll investigate further.

[jwoolston]

@MasDennis Well I'm still a few weeks from getting to it so I will think about it as well. I'll post any ideas/questions here including my intention when I have one if we have not come to a mutual conclusion by then.

[jwoolston]

A few weeks ago #546 was merged in to fix #543. After thinking about it more and moving forward on the scene graph implementation I need to take care of this because thread safety is becoming a bit of an issue and I only intend to allow the GL thread to touch the scenegraph regulated by RajawaliRenderer, so I have no thread safety checks in it.

Here is what I tentatively propose to do:

• Add synchronization checks to mChildren and mPlugins during iteration. getNumTriangles() can realistically be called from a number of places and must iterate over mChildren. From the Java documentation on Collections:

  It is imperative that the user manually synchronize on the returned collection when iterating over it:

  Thus:

  mChildren = Collections.synchronizedCollection(new CopyOnWriteArrayList<BaseObject3D>());
     ...
  synchronized(mChildren ) {
      for (int i = 0, j = mChildren.size(); i < j; ++i) {
         //Whatever;
  }

  While it might not be ideal for allowing removal of objects safely from outside of RajawaliRenderer, it is necessary and correct.

• Use a LinkedBlockingQueue of a class FrameTask or some other appropriate name which has an ENUM of if it is a Camera, child BaseObject3D or Plugin, a flag for add/remove and of course the thing to add remove.

  • This makes it possible to add additional types as the library expands and the need for this occurs (Animation Framework Overhaul #578)

• When changes need to be made users call the various add/remove methods which create an instance of FrameTask with the necessary data and add it to the queue.

• At the start of onDrawFrame(), RajawaliRenderer go through the contents of the queue and performs the necessary tasks, in the order they were called by the user.

• FrameTask would have completely final fields to make them completely thread safe.

• There would be no way to access mChildren, mPlugins, etc directly. In my opinion we should remove any methods which return a reference to these objects. Perhaps returning a copy would be ok if it was absolutely necessary to present this info.

  • By contract these members could only be mutated via these tasks.

• I would also propose the possibility of making these member lists private to ensure that access to them is regulated.

I apologize for not having thought this through in its entirety prior to my camera and concurrency pull requests, but the full extent of how these could cause thread bugs did not come to mind until I got far into the scenegraph.

Please let me know what you guys think.

[MasDennis]

It sounds like a well thought out solution to me.

I agree on restricting the access to mChildren etc. Maybe not ideal but necessary for keeping things safe.
We could return a copy of these collections. I think the name of the getter method for this should reflect the fact that it returns a copy to avoid any confusion.

I like FrameTask. Will you store the objects to be added/removed in a collection of plain Objects or will you create a new interface for this? If so, will this interface store the type of object (the ENUM you talked about)?

[ToxicBakery]

[jwoolston]

@ToxicBakery When I wake up in the mornings now I check my phone for the usually two dozen or so GitHub related emails. When I read this one I couldn't stop laughing for a good 5 minutes. I feel like I have been coming across as a thread-safety warlord, and that that picture really was beyond amusing.

@MasDennis I will change the getters to return copies and document them clearly (including names). As for POJO vs. Interface I had not decided. If I store them as objects, I need to cast them up to their types to store them internally and the ENUM would be a definition in FrameTask. If I make an interface, yes the ENUM definition would go there and I would still need to cast them for storing in RajawaliRenderer. Since there is no way around the casting (that I can see) I'm inclined to add the interface because it makes the intention much more clear and requires developers who might add further to abide by the same rules.

[ToxicBakery]

@jwoolston Glad I could help. Crusades are fine, I have gone on several memory crusades in my history of interaction with Rajawali so you'll hear no complaints from me.

For FrameTask, I am trying to follow in what the conversation is about. My understanding of FrameTask is work to be done when the time is correct in the life cycle. Such work would be adding/removing of objects, movements, changing textures, etc. Is this a correct understanding for the intent of FrameTask? If this is the case, then all I can say is o.O. You have your work cut out for you as that, running it through my head really quick, seems like it would involve a LOT of work.

[jwoolston]

@ToxicBakery only adding/removing of things. Stepping the animations will still be left to however the replacement animation framework handles it as I did not want to clutter/convolute things too much. The big issue here that I am trying to fix is making sure that these lists which need to be iterated are controlled more carefully so that the library can handle the thread safety needs of the library. As long as the animations are stepped by the gl thread either before or after (I vote for after) the frame tasks are performed, everyone will be happy I think. Texture changing I had not considered, I will add that as well. If you think the stepping of animations should be added in, I am open to it. This is pretty extensible so it would be pretty simple to add new tasks, which is something I was going for.

As into crusading for thread safety as I am right now, I am conscious of the performance hit it can cause if performed unnecessarily, which is why I am inclined to move all of it into RajawaliRenderer where it can be more concisely implemented. If there is any input on the matter, please send it my way.

[ToxicBakery]

Well instead of standalone frame task class, what about FrameTask instead being AFrameTaks which can be extended by ATransformable3D.

I only suggest this as more classes = more memory and more garbage collection creating and destroying objects. Since we can not avoid creating ATransformable objects, it may be good to attach code via an abstract super class that performs these general FrameTask actions. Additionally in the future FrameTask could be extended by Animation/Texture/Material classes as needed should we decide it to be necessary.

[jwoolston]

@ToxicBakery Yea I can see the logic in that, it also simplifies the task class. Do you see any issues with allowing mutability to the task to be performed?

[ToxicBakery]

I prefer mutability in 'automated' processes. I think it will provide greater flexibility down the line. Granted your proposed implementation would still allow for mutability by extended the FrameTask class.

[jwoolston]

I am all in favor of reducing garbage. I am leaving a AFrameTask as a member of the package rajawali.renderer and will make its methods package private. The whole point behind doing this is to make sure that only RajawaliRenderer gets to control this process and a user simply requests that a particular thing be performed. This will make sure that they can't interfere with the correct operation, and the parameters can be mutable members.

[jwoolston]

It is technically possible with a LinkedBlockingQueue or ConcurrentLinkedQueue to feed the queue at the same rate it is being consumed, which would result in a perpetual loop of tasks. To prevent this I was going to synchronize on the loop explicitly (not using its protection) which is overkill. So there are two alternatives:

1. Use a simple LinkedList externally synchronized when modifications are made, including executing the list, so that only the items added prior to the current frame get executed.
2. Stick with the ConcurrentLinkedQueue and allow only a max number of tasks to be executed per frame.

Number 1 seems more generic to me, allowing users to sacrifice frame rate as they see fit. 2 would keep things more consistent frame to frame when there was a lot to be done. It is not as universal though, but could potentially be made so by using an AtomicInteger to store and allow adjustment of the maximum count per frame.

Your thoughts?

[jwoolston]

Also, should the various has methods be protected or public? hasChild() hasPlugin() etc. Currently they are mixed.

[ToxicBakery]

It should be whatever makes the most sense for each particular method. I don't think there is any overarching answer.

[jwoolston]

Its a work in progress but if anyone would like to take a look I'd like some input. I feel like there is a lot of repetitive code but I'm pretty sure there is no way around it.

[jwoolston]

Sorry, branch is here: https://github.com/Tenkiv/Rajawali/tree/frame_task_queue

[jwoolston]

I was thinking I could make the addChild/Plugin/Camera and removeChild/Plugin/Camera etc methods queueAddTask() queueRemoveTask etc which would take the various things, figure out the appropriate task values and go from there. This would be a breaking change what is probably one of the most commonly used methods though.

[jwoolston]

@ToxicBakery Ian you had some input on the camera methods when I first added multiple cameras so I'd like your input on an issue here. With the task queue, there is no way to correctly return the index a camera will be added at, so the method getCamera(int) serves little to no purpose. Should I just ditch it? I added a method getCamerasCopy() which returns a shallow copy of the camera list. Would there be a need for getCamera(int)? Currently I have the replace task set to take an index of the item to replace. This is particularly useful for the plugins because of their order sensitivity. Should I also add an ability to replace by specifying the item to replace? It certainly would not be thread safe to assume that the index an item was at in a list copy would stay the same long enough use it for a task.

[ToxicBakery]

I think realistically most applications would only have a few cameras so if the developer was tasked with keeping track of the cameras themselves, I believe this would be acceptable.

If you go this route, then yes, removing int returns are pointless. Actually the entire getCamera method group would not be necessary I believe. Instead the only methods would be the following.

addCamera(ACamera camera);
removeCamera(ACamera camera);

// Might want to use a different name than this but I like 'switchTo' personally.
switchToCamera(ACamera camera);

[jwoolston]

Ok, I suppose there will just be some tasks which for certain objects are meaningless. I will document them accordingly.

[jwoolston]

I was thinking I could make the addChild/Plugin/Camera and removeChild/Plugin/Camera etc methods queueAddTask() queueRemoveTask etc which would take the various things, figure out the appropriate task values and go from there. This would be a breaking change what is probably one of the most commonly used methods though.

@MasDennis and @ToxicBakery I would really love to hear your input on my question above. I am concerned about adding a bunch of similar methods, but don't want to explicitly make this change as it will break a lot of initScene() calls.

[ToxicBakery]

I'm 100% for breaking changes that make sense. In the case of SceneGraph it makes sense in my opinion as it is a better direction for the engine. I am considering changes like this for animations in the future as the current animation implementation and the rewrite both leave something to be desired that I think I can improve on.

Anyways I try to always look at problems from the implementation point of view. That is, I write some suedo code that represents what my proposed internal implementation will support. Based on the suedo codeI can judge if the implementation makes sense. Is it easy to use, is it easy to understand? If it does not score well against either of those questions it would be good to consider changing the internals until the suedo code becomes something enjoyable to write and understand. I know this is not a specific answer and more of a process but I think you can use it to answer your question. Write a sample of how you would use it and then decide if you are happy with that.

[jwoolston]

Thanks. That does pretty well answer the question. This is the first time I am being allowed to contribute so much to a repository that is not private to the company so I just want to make sure I follow with the direction everyone else is interested in having it go.

[ToxicBakery]

I'm pretty sure the only rule here is that whoever breaks it has to buy the first round.

[jwoolston]

Well I think @daveHD and I are the only two people remotely close. When I butcher it I guess I'll need to fed ex a box of newcastle then.

[MasDennis]

@jwoolston Rajawali has so much activity lately that it is hard to keep track of everything that's going on :)

I am in favor of the changes you proposed. I agree with @ToxicBakery that breaking stuff is acceptable when it benefits the framework in the long run. Although we'll probably have to think about release management as opposed to ad hoc updates. It is probably a good idea to set up a dev branch.

The buying rounds rule sounds great. Very expensive for me though since most contributors are based in the US :)

I have to say that American micro breweries are producing a lot of great beer! I was in Montana a few years ago and had several local beers in a place called Whitefish (can't remember the names but they were tasty). I also had a few good ones in Brooklyn a few months ago. Getting thirsty now! Anchor Stream, Arrogant Bastard Ale, Sierra Nevada Pale Ale ... yummm ..

Excessively off topic.

[jwoolston]

@MasDennis Yea I think a few crusades have been initiated. It's good to see though, I feel like I picked the right engine.

[Davhed]

I can supply the microbrew. We have tons of it here in California.... unfortunately all these changes are above my paygrade or I'd be happy to lend more help.

[jwoolston]

@MasDennis @AndrewJo and anyone else who knows. I could use some input on how textures work. I have never tried fiddling with textures after initScene(). What happens if you do? Do they need to be included here?

[Davhed]

I think they should be included, there are a number of unexpected issues with updating textures on the fly in my experience.

[jwoolston]

@Davhed Oi....now I have to figure out how to make that happen with the 30 or so possible method calls in mTextureManager....looks like I need some ENUMS.

[MasDennis]

@jwoolston Yes. The texture manager and its methods. It has grown "organically" so to speak :). This is something that has been bugging me for some time. I've never found the time to tame this beast.
Enums and a TextureConfig object sound like a good solution here.

Basically we have the same problems with textures & concurrency. There's also the problem that the textures cannot be updated during a render cycle. This is catered for in the texture manager though.

I think I'll pick this one up. This has higher priority than md5 animation. Let me have a think about this. I'll come back to you with proposed changes this evening.

[AndrewJo]

Part of it is my fault when I added all those method calls for different texture compression types. I think we should probably have an abstract class which others extend from so we could have something like BitmapTexture, ETCTexture, PowerVRTexture, S3TCTexture classes, etc. which might help organize the API.

[MasDennis]

Yes, either an abstract class or a TextureConfig object with enums. This way we can encapsulate properties like mipmap, recycle, filtertype, wraptype, etc in one class. This way we can reduce the number of methods in the texture manager.

Or maybe we could do a combination. The user would never create instances of PowerVRTexture, S3TCTexture etc themselves but this would be done by the texture manager.

Maybe something like

TextureConfig config = new TextureConfig();
config.setMipmap(true);
config.setRecycle(false);
config.setCompressionType(CompressionType.NONE); // default
// or
config.setCompressionType(CompressionType.S3TC);

mTextureManager.addTexture(Bitmap bitmap, TextureConfig config);

Any thoughts?

[AndrewJo]

For compressed textures, we can't pass in a Bitmap object. It needs to be a ByteBuffer. Also, each compression methods use vastly different configuration options (see #311). Do you have any idea as to how we could reorganize the framework?

[MasDennis]

Of course. Let me have a think about this. I'll look at it tonight.

[jwoolston]

@MasDennis, alright. I'll bundle everything else up in the meantime. I was headed in the direction of a package class like you said, but only for the task queue.

[jwoolston]

@MasDennis (@AndrewJo ... you get a tag because it affects your plugins also) another interesting issue here is what to do about the IPostProcessingFilter. I had a similar problem with plugins, but basically I am just enforcing that all plugins MUST inherit AFrameTask, either through APlugin or by doing it themselves, otherwise it wont be able to be added to the queue. I can't really see how to do this for IPostProcessingFilter because all of the filters inherit from AMaterial essentially and Java does not allow multiple inheritance like C++ 👎 Anyway, some thoughts? I could just make AMaterial inherit AFrameTask but I'm not sure that really makes a lot of sense.

[AndrewJo]

Can't you make it IFrameTask? 😉

[jwoolston]

I can, but there are 3 members that all tasks must have in one form or another.

[AndrewJo]

If you make it an interface, wouldn't you be able to guarantee all tasks will have those 3 members? I'm guessing you have some implementations in the abstract class which is why you went with abstract class in the first place.

[jwoolston]

I do have some implementations in the abstract yes, but its not that big of a deal. Sorry, my OOP terminology was flawed...there are 3 fields.

[jwoolston]

I suppose I indirectly enforce having those fields through the related methods, I just find it inconvenient that they need to be defined in each implimentation.

[jwoolston]

As much as I love the picture in this thread, #651 brings this full circle.

[Davhed]

Epic work Jared!

[jwoolston]

Thanks! There's more where that came from!

[MasDennis]

Great stuff. Thanks for this!
Not to worry though, I'm sure @ToxicBakery has an image for every crusade you initiate.

[ToxicBakery]

I'll try to keep up.