Rajawali / Rajawali

Android OpenGL ES 2.0/3.0 Engine
https://rajawali.github.io/Rajawali/
Other
2.35k stars 700 forks source link

Object picking for augmented reality. #1634

Closed conect closed 7 years ago

conect commented 8 years ago

Hi I've got a Renderer which inherits from https://github.com/googlesamples/tango-examples-java/blob/8d315d38007265e45a4089379a3fc7e6da4ee18e/java_examples_utils/app/src/main/java/com/projecttango/rajawali/ar/TangoRajawaliRenderer.java . Now I want to implement object picking in the augmented reality view. Problem is that the object picking works fine, as long as the backgroundsquad is not visible (mBackgroundQuad.setVisible(false)) but as soon as the squad is rendered, objectcolorpicking doesn't work anymore. Could someone tell me why?

Edit: To reproduce do following:

  1. Clone https://github.com/conect/tango-examples-java.git
  2. Checkout objectpickingsample
  3. Run the augmented reality example.

If you click on the green cube there should be a debugmessage, but there isnt.

Thanks a lot.

jwoolston commented 8 years ago

As I mentioned in the other ticket, you need to be more descriptive with your issues. I have no idea what other stuff you have added to the scene but a screen quad should be thought of as holding up a drawing in front of the camera. The renderer they added there is simply streaming a texture to rajawali from the tango subsystem, not providing geometry to rajawali - or at least not to that renderer.

conect commented 8 years ago

Yes the image frame is supposed to be merely visible in the background. I've placed one green cube in the scene. And registered it for object picking. This works fine as long as the screen quad is not visible.

jwoolston commented 8 years ago

You mentioned using ray picking. Are you doing this yourself or with the RayPicker class? The ray picking in rajawali is incomplete and hasn't been touched along with the other upgrades so I'm suprised if it works under any conditions. Have you tried using color picking? I say that but the color picking in the Rajawali version they used in their examples is broken as well and was only recently fixed.

conect commented 8 years ago

Sry i use object colorpicking. Thouhght it was the same principle...

jwoolston commented 8 years ago

Color picking and ray picking are different algorithms. The color picking in that version of the library is known to be broken. I will be sending them a PR for updating to the latest version of rajawali sometime this week but I do not know if/when they will accept it. In the meantime, you could always force the update yourself.

conect commented 8 years ago

Yes of course i'm already using the newest version now the colorpicking works as long as the screenquad is not visible.

jwoolston commented 8 years ago

I will have to try this out and see if i can reproduce the problem then to figure out whats going on.

conect commented 8 years ago

Thx a lot!

If there's another way for an augmented reality app making use of objectpicking i'de be glad to learn about it of course.

Would it perhaps be possible to render the screen quad in another scene? Edit: Actually that's not an appropriate solution for my issue because i want to render the objects in a virtual scene as well as in an augmented scene.

conect commented 8 years ago

Could you perhaps tell me, how I should tackle this problem. Should I wait till someone like you or other persons who have deep knowledge of the rendering process deal with it and suggest a solution? Or should I search for totally different approaches like several scenes synchronizing the same rendering content?

jwoolston commented 8 years ago

That is entirely dependent on your needs and timeline. I didn't realize their example was actually just library so I need to create an example to demonstrate the same problem before I can look into it further, and I probably wont have time for that until Friday. If you want to zip up and email your project that demonstrates the issue, that will allow me to look at it tonight instead, but otherwise the decision is up to you. If this is a genuine bug that I am able to reproduce, it will receive immediate attention on my part.

conect commented 8 years ago

I guess it's got something to do with the rendering process and the readout of the pixels for the colorpicking. But so far I couldn't find the error. Do you have the tango-hardware? In this case I could send you an example where you could get aware of the problem yourself.

jwoolston commented 8 years ago

I do have tango hardware, so a code example will speed things considerably.

conect commented 8 years ago

Do you think it's realistic to get some information about this issue within the next few days?

conect commented 8 years ago

The error must be caused here[https://github.com/Rajawali/Rajawali/blob/850f3841bd79449f8a1e1063d38fc64af532e8bc/rajawali/src/main/java/org/rajawali3d/util/ObjectColorPicker.java]: GLES20.glReadPixels(pickerInfo.getX(), picker.mRenderer.getViewportHeight() - pickerInfo.getY(), 1, 1, GLES20.GL_RGBA, GLES20.GL_UNSIGNED_BYTE, pixelBuffer); Somehow pixelBuffer is always filled with [-1, -1, -1, -1]

jwoolston commented 8 years ago

Do you think it's realistic to get some information about this issue within the next few days?

Yes. It was a busy holiday weekend.

jwoolston commented 8 years ago

You created a new instance of ObjectColorPicker in your AugmentedRealityRenderer class and called it mPicker. This is name masking the one in the base renderer implementation, which means nothing in the rajawali color picking system is being activated.

conect commented 8 years ago

Please reopen.

Of course this is not the problem, I prepared even a sample for you. And as I already told you the problem is pixelBuffer and not name masking. Please take a look yourself!!

jwoolston commented 8 years ago

Your sample could not be imported. I had to create a new project with all the dependencies because your gradle scripts were based on your local file system.

The name masking is a serious problem in your code that I am not going to fix. If you want to fix it and send an updated renderer class, I can take a look further. The masking is preventing Rajawali from ever receiving notifications about the touch events and more importantly is affecting which class is learning about registrations. Your own log statements are how I first discovered this which caused me to inspect your code when I found that issue.

conect commented 8 years ago

Sry but the only appearance related to my own file system was the path of the Android SDK which in my case was autogenerated. It's acutually only an adapted version of the Tango-Java-Code from the google repository. As I told you several times the bug is not caused by the name mPicker but some issues within the rendering process. I will fix the naming then I hope you will be able to reproduce as well. Thanks.

jwoolston commented 8 years ago
def external_lib_prefix = null
if (project.hasProperty("Tango.catkin_devel_prefix")) {
    external_lib_prefix = project.property("Tango.catkin_devel_prefix")
} else {
    // Building in standalone sample form.
    external_lib_prefix = "../../TangoReleaseLibs"
}

repositories {
    flatDir {
        dirs external_lib_prefix + '/aar'
    }
}

dependencies {
    compile 'org.rajawali3d:rajawali:1.1.501-SNAPSHOT@aar'
    compile (name: 'tango_support_java_lib', ext: 'aar')
    compile project(':tango-app')
}

That is from the app module build.gradle file. There are several relative path references there to the tango library module project, so the zip you sent is not fully self contained.

As I told you several times the bug is not caused by the name mPicker but some issues within the rendering process.

The name masking is an issue with the rendering process.

If you want my help stop arguing with me about issues and fix them instead.

conect commented 8 years ago

But I already fixed it (renamed mPicker into myPicker like written in the edit message), please just clone the repository from google: https://github.com/googlesamples/tango-examples-java and replace the augmented reality folder with my version.

If you want my help stop arguing with me about issues and fix them instead.

Sorry for my rude writing. I don't want to argue, but I hope we can concentrate now on the real problem the readout of the pixelvalues.

conect commented 8 years ago

Would be nice if you reopen and mark as issue, so others could find and tackle this bug as well.

Regards conect.

ToxicBakery commented 8 years ago

I'm not sure if this issue is or is not resolved but without a repository demonstrating the issue this is a very time consuming report. Please create a repository that has a comprehensive example of the issue such that it can be fixed and tested if applicable.

conect commented 8 years ago

Please create a repository that has a comprehensive example of the issue such that it can be fixed and tested if applicable.

See edit of first post.

conect commented 8 years ago

Are you able to reproduce the bug now? Sorry if I was offending in my previous posts, I should have written a more specific description of the issue.

conect commented 8 years ago

So this is not going to be addressed or fixed in the near future?

jwoolston commented 8 years ago

So this is not going to be addressed or fixed in the near future?

I have lost the desire to work on this for the time being. I get sick of people assuming I get paid to do this and I do not like having to spend a bunch of time setting up something to validate if the issue exists or not. I will address it at some point. As @ToxicBakery and I have both pointed out having a repository or zip of something that can be imported directly into AS without needing to fuss with things will speed it up considerably. I am not seeing any attachments to this ticket anymore.

conect commented 8 years ago

Ok for me this criticism doesn't seem legitimate. I provided a very simple solution for you to get aware of the problem. Cloneing and running an example like I suggested in my first post, is the maximum I can provide. If you're not willing to spend time on this issue, that's absolutely acceptable, but blaming me for "name masking" is not constructive, you could just have told me that you're busy with other things. What's absolutely fair enough.

conect commented 8 years ago

Did someone try to compile my code? Thanks.

brianPlummer commented 8 years ago

@ToxicBakery I have modified the java_augmented_reality_example to exhibit this bug: https://github.com/brianPlummer/java_augmented_reality_sample

I have tried several things but have not gotten the RayPicker or ObjectColorPicker to work correctly at all.

Here is a link directly to the renderer where i have a very simplistic example: https://github.com/brianPlummer/java_augmented_reality_sample/blob/master/app/src/main/java/com/projecttango/examples/java/augmentedreality/AugmentedRealityRenderer.java#L68

conect commented 8 years ago

@brianPlummer have you found a workaround for this problem?

mananchai commented 7 years ago

I'm having the exact same problem, anyone found a solution to this?

leiseliesel commented 7 years ago

@ToxicBakery @mananchai I ran into the same issue a few days ago. After a little research, I found that the ScreenQuad constructor had been heavily changed. So I used the Rajawali version "1.0.325". Now it's displaying the tango camera preview mirrored on the diagonal of the screen (on the Lenovo Phab2Pro device). The interesting thing is that the OnObjectPickedListener is getting called now. On the first touch event, the OnObjectPickedListener behaves the right way (it calls the onObjectPickedListener when the sphere had been touched. And it does nothing when everything else gets touched). But after the first touch, the OnObjectPickedListener gets called every time the user touches the display regardless of whether the Sphere has been touched or just another position on the screen.

Since the old version (https://github.com/googlesamples/tango-examples-java/releases/tag/v1.31-gemma) of the "googlesamples/tango-examples-java" repository worked without any issues on the Tango Tablet, I guess the ScreenQuad does something like an overlay making the rendered objects unclickable. The mirrored camera preview could rely on the problem, that the x coordinate is based on the height of the Tango Tablet where the width of the Lenovo Phab 2 Pro is related to the x coordinate.

I'm trying to solve this issue, but my knowledge is very limited. Do you have some tips so I can get a better understanding of the problem?

[Edit:] I just tried the following: set rajawali version in gradle to 1.0.325 and adjusted the emerging bugs to that library version (f.ex. Renderer -> RajawaliRenderer). You also shouldn't only register the Augmented Reality Object (like sphere etc) you should also register the ScreenQuad to the ObjectColorPicker.

If you want/need to use a greater version: The screenQuad is always in front of the other objects. Since the ScreenQuad also is an Object3D object and its Position is set to the camera root (0,0,0) it will always be in front of the other objects. The onObjectPickedListener will therefore always only register the event on the ScreenQuad. I just set the ScreenQuad visibility to false everytime a click event has passed and set it back to true in the onObjectPicked and NoObjectPicked method. It's a really bad workaround but since I need that functionality asap it's currently the only one working for me.

If my code is needed, just say so and I'm going to create a repository

adamantivm commented 7 years ago

With the permission of @jwoolston and @ToxicBakery I would like to help find a more permanent solution to this issue. I know more people than who have chimed here are suffering from this limitation. For a limited time, I will have the ability to spend enough time to prepare testable use cases and I don't mind diving into the guts of Rajawali.

jwoolston commented 7 years ago

@adamantivm I'm certainly not going to stop you. If you can put up a clear repository that demonstrates it and that I can simply import to AS without having to go find what repo some dependency is in or being tied to a local machine, I will take a look at it.

adamantivm commented 7 years ago

@jwoolston thank you very much for such a prompt response. I will definitely prepare everything to make it as little work as possible for you guys to test / verify. Hopefully I can provide a readily available fix to go with it as well. What I would most appreciate for you is high-level guidance / direction more than actual bug fixing.

At a first glance and after some initial testing, I have a few ideas but I am unsure if they are acceptable or may have undesired side-effects in Rajawali.

The approach I'm thinking about is to modify the color picking rendering strategy in Object3D.renderColorPicking so that it is possible to prevent screen quads from overshadowing everything else in scenes like the one we have here for AR purposes.

The most trivial way to do that is to simply skip screen quads, like so:

        if (isDestroyed() || (!mIsVisible && !mRenderChildrenAsBatch) || isZeroScale() ||
                this instanceof ScreenQuad)
            // Neither the object nor any of its children are visible
            return;

I tested this and it works for the presented case. A general questions for the experts here is: do you foresee any case where you would like to color pick on the screen quad? Does this sounds like a fundamentally wrong way to go about it?

jwoolston commented 7 years ago

Given the intended use of screen quads, I can not think of any reason why you would ever want to be able to pick them to be honest. The only touch events you would be interested in with a screen quad would be coordinate based, and therefore a different interaction path anyway. I think this is an acceptable solution and it has negligible impact on the greater part of the engine or our WIP for 2.0.

adamantivm commented 7 years ago

Thanks very much, @jwoolston , we have one solution then.

If you don't mind my asking, is there a good reason why depth test is done globally for color picking? This is the reason why ScreenQuads overlay the rest for color picking test. Doing the depth test per object also solves the problem and further allows picking on ScreenQuads (but as you pointed out, not sure who would ever want to do that). Is this for a performance reason? Would doing the depth test per object for color picking purposes be a preferable solution to my initial proposed hack?

jwoolston commented 7 years ago

It is an effort to reduce unnecessary GL state changes mostly. There are other potentially bad side effects of doing it globally and this is one of the things we are seeking to address in 2.0. I'm not sure doing it per object would correct the screen quad issue though due to their use in multi pass rendering.

adamantivm commented 7 years ago

@jwoolston I did test adding this to the line below the comment and it does fix the ScreenQuad issue reported:

            if (!mEnableDepthTest) GLES20.glDisable(GLES20.GL_DEPTH_TEST);
            else {
                GLES20.glEnable(GLES20.GL_DEPTH_TEST);
                GLES20.glDepthFunc(GLES20.GL_LESS);
            }

            GLES20.glDepthMask(mEnableDepthMask);

I am ready to send either this or the first proposed hack in a PR. I am happy to provide any accompanying samples or tests that you require. Which solution would you prefer?

jwoolston commented 7 years ago

We can go with the per object depth testing as it will better handle situations where an object is not rendered to the depth buffer for artistic reasons. The whole premise of color picking is that if the object is obscured for one reason or another, it should not be pickable. As for tests/samples, I am fine with anything as long as it is easy for me to setup and see. I just dont like needing to spend 20 minutes trying to get someones project working because they didn't configure gradle properly (I already hate gradle enough, I don't need reminders of why).

adamantivm commented 7 years ago

Sounds good, I'll go down that path then. I'll do the dirty Gradle hacking for you gladly. It won't be the first (or last) time.

jwoolston commented 7 years ago

Cool, I'll get to it sometime this week. I will be spending a good deal of time with rajawali for work anyway. I should add too that if you want to add the picking demo to the existing VR demo, I am perfectly happy with that.

adamantivm commented 7 years ago

Thank you very much @jwoolston . I ran out of time today but will be submitting everything tomorrow. Also thank you for the offer and I am sorry for the dumb question but, what VR demo are you referring to? I'm working with the Tango Augmented Reality example, which is where this problem manifests itself. I am planning to send a PR to Rajawali with the proposed change, including a link to a fork of said example that adds color picking plus some tweaked Gradle build files to make your life easier to test against my patch to Rajawali - still need to figure out how. I have a preference to be able to test against this particular one example. I may be able to get color picking added permanently to it later on. Please let me know if you disagree with this or if you have any further suggestions. Otherwise thanks once more for your support and hold tight for PRs tomorrow.

jwoolston commented 7 years ago

That will work fine. If you need me to deploy a snapshot from the PR to get the examples Gradle to comply I can.

On Tue, May 23, 2017, 3:02 PM Julian Cerruti notifications@github.com wrote:

Thank you very much @jwoolston https://github.com/jwoolston . I ran out of time today but will be submitting everything tomorrow. Also thank you for the offer and I am sorry for the dumb question but, what VR demo are you referring to? I'm working with the Tango Augmented Reality example https://github.com/googlesamples/tango-examples-java/tree/master/java_augmented_reality_example, which is where this problem manifests itself. I am planning to send a PR to Rajawali with the proposed change, including a link to a fork of said example that adds color picking plus some tweaked Gradle build files to make your life easier to test against my patch to Rajawali - still need to figure out how. I have a preference to be able to test against this particular one example. I may be able to get color picking added permanently to it later on. Please let me know if you disagree with this or if you have any further suggestions. Otherwise thanks once more for your support and hold tight for PRs tomorrow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Rajawali/Rajawali/issues/1634#issuecomment-303545261, or mute the thread https://github.com/notifications/unsubscribe-auth/AC2r5VpLpDkGPJA_N6jhP6QjEtMQ_HpZks5r81dYgaJpZM4H1Y3i .

adamantivm commented 7 years ago

ok, PR #1895 sent

jwoolston commented 7 years ago

Fixed via #1895