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

Updated filament from 1.9.25 -> 1.10.6 #116

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

RGregat
Copy link
Contributor

@RGregat RGregat commented Jul 9, 2021

  • Recompiled all *.mat files
  • Fixed issues because of removed filament APIs.

* Recompiled all *.mat files
* Fixed issues because of removed filament APIs.
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.

Can you also include the ones in ux/assets-sources/materials ?
You can move them directly to core/assets-sources/materials in order to not forget them every time.

Did you made tests to be sure everything is OK?
An Assert error should crash the app if it's not the case.

Can you also have a look at this bug #70 to see if it has been resolved with the latest Filament version?

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

  • I updated the *.mat files from the core and ux module.
  • I tested the filament update on all samples and my personal project.

Can you also have a look at this bug #70 to see if it has been resolved with the latest Filament version?

  • Do you mean the necessity to add the following in onViewCreated?
if (renderer != null) {
    renderer.getFilamentView().setColorGrading(
        new ColorGrading.Builder()
            .toneMapping(ColorGrading.ToneMapping.FILMIC)
            .build(EngineInstance.getEngine().getFilamentEngine())
    );
}

@ThomasGorisse
Copy link
Collaborator

Do you mean the necessity to add the following in onViewCreated?
if (renderer != null) {
renderer.getFilamentView().setColorGrading(
new ColorGrading.Builder()
.toneMapping(ColorGrading.ToneMapping.FILMIC)
.build(EngineInstance.getEngine().getFilamentEngine())
);
}

Exactly!

BTW, nothing related but searching for a way to draw polygons I found your great issue (and own answer) here google-ar/sceneform-android-sdk#443
Thanks a lot, it greatly helped me.
If you are still working on it, I strongly recommend you moving to JTS Topology Suite instead of Poly2Tri for Delaunay triangulation.
It works much better, it's simpler and doesn't forbid intersection vertices.

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

Exactly!

  • I have no idea how to spot the difference within the sample projects, but for my personal project I have still the same coloring issue, when I'm not setting the ColorGrading to FILMIC in onViewCreated.

BTW, nothing related but searching for a way to draw polygons I found your great issue (and own answer) here google-ar#443
Thanks a lot, it greatly helped me.
If you are still working on it, I strongly recommend you moving to JTS Topology Suite instead of Poly2Tri for Delaunay triangulation.
It works much better, it's simpler and doesn't forbid intersection vertices.

Not a bad idea, but I abandoned the mentioned idea. I have a Polygon drawing node which still uses poly2tri. Maybe I change this in the near future! Thanks

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

Can you also include the ones in ux/assets-sources/materials ?
You can move them directly to core/assets-sources/materials in order to not forget them every time.

  • Does it make sense to move them to the core module? I mean the VideoNode expects the compiled *.filamat files in the ux module. So I change the oh I forgot to compile the *.mat files from the ux module, to oh I forgot to copy the *.filamat files to the ux module :D

@ThomasGorisse
Copy link
Collaborator

I have no idea how to spot the difference within the sample projects, but for my personal project I have still the same coloring issue, when I'm not setting the ColorGrading to FILMIC in onViewCreated.

I asked Filament here: google/filament#3909

Does it make sense to move them to the core module? I mean the VideoNode expects the compiled *.filamat files in the ux module. So I change the oh I forgot to compile the *.mat files from the ux module, to oh I forgot to copy the *.filamat files to the ux module :D

Do it as you feel. I'm also not against moving VideoNode to core.

@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented Jul 9, 2021

Even more because I really ask myself if it still make sense to have core, ux, sceneform instead of a unique sceneform module.

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

Even more because I really ask myself if it still make sense to have core, ux, sceneform instead of a unique sceneform module.

true
In general, the package-structure could need an overhaul. Especially the rendering packag is to big, some sub packages would be beneficial.
We should do that in another branch

@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented Jul 9, 2021

In general, the package-structure could need an overhaul. Especially the rendering packag is to big, some sub packages would be beneficial.

An enormous clean would be enormously benefit.
The question is more to know who and when.
Even more because I contacted most of the Sceneform articles writers this week to tell them that we are here and great.
Some answered that they will update their old article sfb code and 1.1.15/1.1.16 dependencies parts.
So It's time to have a clean/fixed version or to keep it mostly safe like it is.

We should do that in another branch

I'm pretty much against having separated branch except for the PRs because it's a nightmare to maintain.

BTW2, some DepthPoint, InstantPlacement, Reticle spoil:

screen-20210709-170542_Trim.mp4

It's working awesomely. I just need to clean it before pushing.

@ThomasGorisse
Copy link
Collaborator

ThomasGorisse commented Jul 9, 2021

It's working awesomely. I just need to clean it before pushing.

It's funny cause I'm actually plastering my wall (In real life ;-)) and I can see the difference while doing it with depth points and the reticle.

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

I'm pretty much against having separated branch except for the PRs because it's a nightmare to maintain.

Same here, i meant not this current PR :)

@RGregat
Copy link
Contributor Author

RGregat commented Jul 9, 2021

An enormous clean would be enormously benefit.
The question is more to know who and when.
Even more because I contacted most of the Sceneform articles writers this week to tell them that we are here and great.
Some answered that they will update their old article sfb code and 1.1.15/1.1.16 dependencies parts.
So It's time to have a clean/fixed version or to keep it mostly safe like it is.

  • II would volunteer to do the cleanup, or at least to try it. But with your announcement to the writers, I think it make sense to wait with such a huge change.

@ThomasGorisse
Copy link
Collaborator

I hope they can now include some evolving code block like a gist displaying our master GitHub samples code.

@ThomasGorisse ThomasGorisse merged commit 0ab85ce into master Jul 10, 2021
@ThomasGorisse ThomasGorisse deleted the refactor/update_filament_to_1.10.6 branch July 10, 2021 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants