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.

Sign up for GitHub

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

Jump to bottom

Add a dialog for customizing FBX import #59810

Merged
merged 1 commit into from
Dec 18, 2022
Merged

Add a dialog for customizing FBX import #59810

merged 1 commit into from
Dec 18, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Apr 2, 2022 •
edited
Loading

  • If FBX files are found, a dialog will pop up asking to configure FBX2GLTF
  • Dialog can also be accessed by going Editor -> Configure FBX Import
  • The dialog also shows a link to click to download the converter, which should contain instructions.
  • Noticed LinkButton does not allow specifying an url, so added this (supersedes Add a uri property to LinkButton #30675 )

Note that the link is non functional yet. The website needs to be created.

image

@reduz reduz requested review from a team as code owners April 2, 2022 11:55
bruvzg
bruvzg reviewed Apr 2, 2022
View reviewed changes
editor/fbx_importer_manager.cpp Outdated Show resolved Hide resolved
editor/fbx_importer_manager.cpp Outdated Show resolved Hide resolved
@reduz reduz force-pushed the fbx-import-dialog branch from 31a0827 to f87a195 Compare April 2, 2022 13:46
@fire-forge
Copy link
Contributor

Noticed LinkButton does not allow specifying an url, so added this.

Does this supercede #30675 then?

@Calinou Calinou added this to the 4.0 milestone Apr 2, 2022
@reduz
Copy link
Member Author

reduz commented Apr 3, 2022

@fire-forge yeah I guess

fire
fire approved these changes Apr 3, 2022
View reviewed changes
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Reviewed the overview design and it looks good. Did not go over in fine detail.

fire
fire reviewed Apr 3, 2022
View reviewed changes
scene/gui/link_button.cpp Outdated Show resolved Hide resolved
Calinou
Calinou previously requested changes Apr 3, 2022
View reviewed changes
editor/fbx_importer_manager.cpp Outdated Show resolved Hide resolved
editor/fbx_importer_manager.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga mentioned this pull request Apr 7, 2022
Closed
This was referenced Apr 9, 2022
Closed
Merged
akien-mga
akien-mga reviewed Apr 15, 2022
View reviewed changes
editor/fbx_importer_manager.h
#include "editor/editor_file_dialog.h"
#include "scene/gui/dialogs.h"

class FBXImporterManager : public ConfirmationDialog {
Copy link
Member

Choose a reason for hiding this comment

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

This class is only relevant of the FBX importer exists, so it should go in the modules/gltf/editor/ folder together with the importer.

Copy link
Member

Choose a reason for hiding this comment

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

The only thing I don't know yet how to do if moving this as I suggested, is how to the entry in the Settings menu.

@akien-mga akien-mga mentioned this pull request Apr 29, 2022
Closed
akien-mga
akien-mga reviewed Apr 29, 2022
View reviewed changes
editor/editor_node.h Outdated Show resolved Hide resolved
@fire
Copy link
Member

fire commented Jun 7, 2022

Need a rebase.

@reduz
Copy link
Member Author

reduz commented Jun 23, 2022

@fire feel free to take it over if you want

@akien-mga akien-mga mentioned this pull request Jul 26, 2022
Merged
2 tasks
@YuriSizov YuriSizov mentioned this pull request Aug 3, 2022
Open
@fire
Copy link
Member

fire commented Aug 24, 2022

@akien-mga I think I need to remake the branch or can I edit reduz's branch?

@YuriSizov
Copy link
Contributor

@fire If you want, you can push to reduz's fork. We just need to be sure he doesn't delete it 🙃

@fire fire mentioned this pull request Sep 7, 2022
Closed
@Calinou Calinou mentioned this pull request Oct 6, 2022
Closed
@fire
Copy link
Member

fire commented Dec 7, 2022

I completely forgot about this code.

@akien-mga
Copy link
Member

I'm updating #30675 to get it merged, I'll rework this PR then.

@akien-mga akien-mga marked this pull request as draft December 17, 2022 14:58
@akien-mga
Copy link
Member

It's not perfect but it should be good enough to merge, in parallel with godotengine/FBX2glTF#27 and godotengine/godot-website#392.

@akien-mga akien-mga marked this pull request as ready for review December 18, 2022 00:07
@reduz @akien-mga
Add a dialog for customizing FBX import
e93d991 
* If FBX files are found, a dialog will pop up asking to configure FBX2glTF.
* Dialog can also be accessed by going Editor -> Configure FBX Import.
* The dialog also shows a link to click to download the converter, which
  should contain instructions.
akien-mga
akien-mga approved these changes Dec 18, 2022
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Let's merge this for now, can be improved further later on.

@akien-mga akien-mga requested a review from Calinou December 18, 2022 11:25
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga merged commit 02f24eb into godotengine:master Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanfranke nathanfranke nathanfranke left review comments

@bruvzg bruvzg bruvzg left review comments

@fire fire fire approved these changes

@akien-mga akien-mga akien-mga approved these changes

@Calinou Calinou Awaiting requested review from Calinou

Assignees
No one assigned
Labels
enhancement topic:editor topic:import topic:3d
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
8 participants
@reduz @fire-forge @fire @YuriSizov @akien-mga @Calinou @bruvzg @nathanfranke