Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise filament version to 1.9.25 #68

Merged
merged 3 commits into from
May 24, 2021

Conversation

fvito
Copy link
Contributor

@fvito fvito commented May 21, 2021

Hi!

First of all, thanks for this library, it has saved me lots of time and headaches.

I'm not sure if this sort of PR are accepted here, if not, I'm sorry, but I've noticed that the library is using an older version of filament, which has been recently updated. There were some OpenGL memory leak fixes in the last couple of version so I figured It would be nice to include thos here too.

From my testing, all of the samples apps worked (apart from augmented images - I'm not sure how that one is supposed to work).
I've also did some profiling tests and it seems to be a bit better. I've looked at the source code, but (I might be wrong here) it seems like all the materials and textures should be cleaned up by native filament implementation, so I guess there might still be some leaks in there.

@ThomasGorisse
Copy link
Collaborator

Hi,

Thanks for your share. Every PR are welcome here.

Updating to the last Filament version was in the pipe but for that we need to generate material resource files with the latest version of the Filament Tool = .mat files in resources folder must be compiled to . filamat files and added to assets folders.

Can you do it in your PR?

Thanks

@fvito
Copy link
Contributor Author

fvito commented May 22, 2021

Sure, I can do that
I've compiled them with the default flags as i was not sure what config should be used.
Let me know if I should compile them again with different config

@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented May 22, 2021

That's perfect.
I don't remember the exact parameters but the defaults ones on the Filament github pages with -mobile should be the good ones.
I gave you contributor access so you can PR from this repo branches instead of having to fork it.(You can do everything from Android Studio or the GitHub desktop app)
Concerning the Augmented Image sample, just display the images from the res/drawable folder on your computer or put it in the emulator settings.
If everything is ok, we will release after this commit.
Thanks a lot.

@fvito
Copy link
Contributor Author

fvito commented May 23, 2021

Thanks. I've recompiled the materials with -p mobile this time. All the examples worked so I think everything is ok now.

Copy link
Collaborator

@ThomasGorisse ThomasGorisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect

@ThomasGorisse ThomasGorisse merged commit 331c299 into SceneView:master May 24, 2021
@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented May 24, 2021

Thanks a lot @fvito. The latest version of Filament seems awesome !!!!
I didn't see any issue displaying a quite important number of assets at the same time.

@grassydragon also made a great PR for the plane detection rendering.

I think we only have a little issue with the camera stream rendering which seems a little laggy.
Maybe some code treated in the wrong thread. I'll try to have a look.
If you see something about it, it will be great for this release (Optimisation)

Thanks to both of you.

@fvito
Copy link
Contributor Author

fvito commented May 25, 2021

Hmm, I haven't noticed any lag on my device (OnePlus 7T) although I did only have a simple scene.
I did notice that colors from camera look a bit weird, specially the blues, If I'll have some time in the weekened, I'll take a look at what is going on with the camera colors.

@grassydragon
Copy link
Contributor

Hi, @fvito!
Yes, there is actually the same problem as here: #60

@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented May 25, 2021

With the glTF sample, don't you see any lags after about 5 tigers on the Scene and moving to a different place.
The camera become a very little laggy on my tests.
I know it's quite minor and even more since I can mostly see it when the camera makes quick movements but since this is probably happening in the FrameCallback, it can have a very bad impact for the rest of Sceneform.

If anyone see something to much consuming from the SceneView.doFrame(), it will be very helpful before releasing.

I have checked all the Filament Swapchain and model rendering part and everything looks goods with the latest Filament version.

@grassydragon Can you have a look to check if there is no issue on the plane detection/controller PlaneVisualizer?

@fvito Can you have a look at the CameraStream specially the near/far plane and resolution?

@imbrig, @VojtaMaiwald Any idea ? Can you test the latest version?

I'm on the Renderer part.

Thanks

@ThomasGorisse
Copy link
Collaborator

Hi, @fvito!
Yes, there is actually the same problem as here: #60

That's right.
Can you make a PR with the @romainguy answer ?

@grassydragon
Copy link
Contributor

With the glTF sample, don't you see any lags after about 5 tigers on the Scene and moving to a different place.

No, I can't see any lags.

Can you make a PR with the @romainguy answer ?

I'm just afraid that the new tone mapping will look differently in existing Sceneform applications and I hope that the problem will be fixed soon.

@ThomasGorisse
Copy link
Collaborator

The release is online. I made some little improvements concerning the rendering.
There is still some little more optimisations that could be done but I think that they mostly must done on the entities transformations update on the FrameCallback.

@fvito
Copy link
Contributor Author

fvito commented May 26, 2021

Hi, @fvito!
Yes, there is actually the same problem as here: #60

In my case it was the actual camera feed that was showing this color errors. But I have noticed the same with another SDK that also uses Filament so I guess since camera feed is rendered as a texture, filament is applying the tonemapping to it aswell, causing this issues?

I'm just afraid that the new tone mapping will look differently in existing Sceneform applications and I hope that the problem will be fixed soon.

What if we introduce a property on the SceneView for changing the tone mapping? From what I currently see, the ToneMapper class is not exposed - or I've missed it. That would make it opt it, so existing implementations can still use the tone mapper, but it would give us an option to change/disable it in certain cases

But yeah, lets hope the fix it soon, they seem to be quite active with the development, a new version was already released 😃

@grassydragon
Copy link
Contributor

In my case it was the actual camera feed that was showing this color errors. But I have noticed the same with another SDK that also uses Filament so I guess since camera feed is rendered as a texture, filament is applying the tonemapping to it aswell, causing this issues?

Actually, the problem is not in the tone mapping. Filament uses the tone mapping to render the scene in HDR and it can be disabled using the LINEAR tone mapper. Another feature of Filament is that it uses colors in the linear space in all calculations and converts them to the sRGB space before showing them on the screen. Filament expects colors to be in the linear space and colors in the sRGB space can be converted as described in the Filament manual. However, Filament doesn't have a function for converting colors but has the inverseTonemapSRGB function that can be used for quickly converting colors while canceling the tone mapping as a side effect. The problem is in the inverse tone mapping function and we can avoid it by converting colors manually as suggested in the Filament manual.

What if we introduce a property on the SceneView for changing the tone mapping?

I think that since the Filament engine instance can be acquired from Sceneform it is possible to change the tone mapping to FILMIC in your application code.

@romainguy
Copy link

The problem is in the inverse tone mapping function and we can avoid it by converting colors manually as suggested in the Filament manual.

It depends on what you are doing. If the tone mapper is set to LINEAR then all you need indeed is to apply the EOTF to go from sRGB to "linear sRGB". If your tone mapping is not LINEAR then you'll also want to apply the inverse tone mapping function to preserve the camera stream as-is.

In general I believe it would be useful to give applications a way to access the ColorGrading API, which includes tone-mapping. This API belongs to View so accessing the Engine is not enough.

@grassydragon
Copy link
Contributor

Thank you for correcting me!
I actually wasn't looking at the Filament API reference so we will need to check if access to View is provided.

@ThomasGorisse
Copy link
Collaborator

Correct me if I wrongly understood but to summarize we have to:

  • Give access to the Filament View.colorGrading which is currently defined in the Renderer class
  • Wait for the Out-of-gamut colors produced by inverse tone mapping cause hue shifts fix
  • Workaround the problem for now maybe at the samples level since it should be fixed for the default ACES_LEGACY on future Filament versions by forcing the FILMIC tone mapping like this:
view.setColorGrading(new ColorGrading.Builder()
            .toneMapping(ColorGrading.ToneMapping.FILMIC)
            .build(EngineInstance.getEngine().getFilamentEngine()));

@grassydragon If it seems OK for you, can you make the PR ?

Thanks

@grassydragon
Copy link
Contributor

Yes, I think it is the best solution right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting error when applying material colour to 3d model . Incorrect colors in ViewRenderable
4 participants