Please also amend thirdparty/README.md to properly document the provenance of the thirdparty/assimp_fbx/ files, which commit they're from, and how to sync with upstream.

@akien-mga This can never ever be sync'd with assimp this is our own parser there is literally no way to merge them, I will mention this in the readme. It is heavily rewritten in some areas and can't be patched from assimp.

We heavily customised the document parser to remove:

• third-party dependencies
• no longer returns ANY assimp types, uses Vector3, Vector2 and Transform where relevant.
• bad code / unsafe memory access
• assertions
• assimp logging
• simplified it
• many other things

Will get on the rest of the review stuff though thanks, I appreciate you spending time doing this, as it's a lot to review I will get on these today.

The only reason it still exists in third party is the licensing requirements.

OK ready for review again @akien-mga

Tested with bistro too @reduz so should be good :)

Will put a trailer out for this too. (video/YT)

Removed some dead code I found while I was doing my own review

Static checks are gonna fail this is WIP now for the refactoring required

Okay ready for round 2 reviews! @akien-mga @reduz

Additionally to my last review, please also update COPYRIGHT.txt in the root folder with:

diff --git a/COPYRIGHT.txt b/COPYRIGHT.txt
index e72cbf606b..2cd1c6a63f 100644
--- a/COPYRIGHT.txt
+++ b/COPYRIGHT.txt
@@ -55,6 +55,13 @@ Comment: Godot Engine logo
 Copyright: 2017, Andrea Calabró
 License: CC-BY-3.0
 
+Files: ./modules/fbx/fbx_parser/
+Comment: Open Asset Import Library (assimp)
+Copyright: 2006-2016, assimp team
+ 2007-2020, Juan Linietsky, Ariel Manzur.
+ 2014-2020, Godot Engine contributors.
+License: BSD-3-clause and Expat
+
 Files: ./platform/android/java/aidl/com/android/vending/billing/IInAppBillingService.aidl
  ./platform/android/java/res/layout/status_bar_ongoing_event_progress_bar.xml
  ./platform/android/java/src/com/google/android/vending/expansion/downloader/*
@@ -110,11 +117,6 @@ Copyright: 2007, Starbreeze Studios
  2014-2020, Godot Engine contributors.
 License: Expat and Zlib
 
-Files: ./thirdparty/assimp/
-Comment: Open Asset Import Library (assimp)
-Copyright: 2006-2016, assimp team
-License: BSD-3-clause
-
 Files: ./thirdparty/bullet/
 Comment: Bullet Continuous Collision Detection and Physics Library
 Copyright: 2003-2013, Erwin Coumans

Done :)

Fixed file-format (i added newlines to make the file width better for git) in README.md

Thanks a ton for this huge and amazing improvement!

<3 so happy

Yes, awesome job!!!

Any of this going to be upstreamed to mainline assimp?

great work !, does gltf also depend on assimp ? and if so is there going to be a rewrite for gltf aswel ?

Why was this PR merged into 3.2 instead of master? This is a huge change so it's extremely likely to cause regressions (and it has). Such things are not really appropriate for a stable branch. Additionally, regardless of whether it would be accepted into 3.2, PRs should target master as that's where development happens and the source of all future stable release branches (see here).

@aaronfranke Because we decided to make an exception. I know the policy, I wrote it.

There's a matching PR for the master branch: #41985. We trust that @RevoluPowered will keep updating it to match the latest state of the 3.2 PR, fix master-specific issues, and additionally add fixes for any regressions that will have been identified in the 3.2.4 beta builds.

Godot 4.0 changes things significantly around the import pipeline, which makes the two versions of the PR difficult to keep in sync at all times, and there are outstanding issues that @RevoluPowered needs to solve in the master branch. On the other hand, the 3.2 branch is where all the main development was done for this feature over one year, and has been heavily tested in production on hundreds of models. The master branch is too unreliable to even do reliable testing of asset imports due to many bugs related to its pre-alpha status. Finally, 3.2.4 is planned to be a feature-heavy release with a longer development cycle, and many similarly big features have been merged like GLES3 batching, which will imply several beta builds, and plenty of time to iron out issues created by this PR.

TL;DR: @reduz and I agreed to make an exception to the policy. That doesn't change the policy.

Why should I have to justify release management decisions all the time?

Geez guys, can you have a bit more common sense instead of wasting time discussing rules for the sake of it? FBX is almost unusable in its current state. Anything not a box will most likely break when you import it.

This PR fixes pretty much every problem and lets you import massively complex scenes without issue. Yes, it may break compatibility a bit, but do you think any user really cares about that when in exchange they can have an importer that will work with anything they throw at?

It also needs testing as soon as possible because there is a company paying for it that will fix any further problem that users have wit it. If we wait until 4.0 is out, it may no longer be the case by then. This is why the PR is aimed mainly at 3.2 and then it will be ported to 4.0 as an exception.

Really, just try to make an effort to see the whole picture before going into pointless rants. This is not the first time it happens, so If you need us explaining you the decisions all the time while others seem to grasp them fine, either try to get more involved in the development process so you can understand them better too or, if this is not possible, then at least try to assume that those deciding on these things have a good judgment and trust them. If you have any doubts about it, simply ask them privately or better yet, on IRC,, so any other contributor can answer you too.

This PR fixes pretty much every problem and lets you import massively complex scenes without issue. Yes, it may break compatibility a bit, but do you think any user really cares about that when in exchange they can have an importer that will work with anything they throw at?

The same can be said about the new navigation system, but this wasn't backported to 3.2.

@akien-mga As you said, this is an exception. I am not questioning whether or not you know the policy you wrote, I am questioning the exceptions. The explanation you gave is perfectly fine.

@reduz I don't really see the problem with asking this publicly on GitHub, other people may have the same concerns and not everyone uses IRC. For archival purposes, it's also much more difficult to dig up an IRC log instead of just looking at the PR. Sometimes @akien-mga posts IRC snippets on GitHub, perhaps this should be done more often?

The navigation system, while not really usable for complex use cases in 3.2.x, is still relied on by many people in production for various needs. The new NavigationServer API isn't quite comparable at all, so upgrading can take a while. It's a bit different from the current FBX support, as I believe nobody was relying on it beforehand.

@Calinou The new navigation system can be used in roughly the same manner as it is today. Bake navmesh -> call get_simple_path(), handle how you see fit. You even set up the scene tree in the same manner if you so choose. It just also provides much much more. There may be a few API changes, but the benefits far outweigh the cost in my opinion.

With the announcement that 4.0 will only have Vulkan, users who cannot use Vulkan and will be stuck on 3.x version will not have this until they can update to 4.1 which will be 2022 at best.

This is not the topic, but I don't remember ever saying that the new NavigationServer API couldn't be backported to the 3.2 branch. On the other hand, the current implementation fully broke compatibility with the previous API and cannot be backported as is. It's also not something that I would work on myself.

So if a contributor wants to make a backport that doesn't break compatibility for 3.2 users, then it can be considered to be integrated in a 3.2.x release eventually. (Not making any promise here, but it can be assessed in the PR.)

Is embedded collision support being planned? Example, mymodel, mymodel_collision .. mymodel2, mymodel2_collision and so on.,,,, will embedded standard pbr maps be detected automatically?

Is embedded collision support being planned? Example, mymodel, mymodel_collision .. mymodel2, mymodel2_collision and so on.,,,, will embedded standard pbr maps be detected automatically?

The FBX importer currently doesn't have any active maintainer, so I wouldn't expect new features to be added to it anytime soon (not for 4.0 at least).

@Gusher Not FBX, but I implemented support for importing and exporting physics collisions in GLTF in #69266. This may be suitable for your uses if you want to store physics information in a 3D model file.