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

Document editor import options in the class reference #49524

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 12, 2021

Tooltips are displayed when hovering import options, both in the Import dock and in the import defaults editor (which is in the Project Settings).

This closes godotengine/godot-proposals#2041. See also #48548 and #49525 (can be merged independently).

Preview

Disregard the black text – it's not a bug related to this PR.

Import dock

Import dock

Import defaults editor

Import defaults editor

TODO

  • Only one importer is present in the documentation right now. Add the rest of the importers.
  • Write documentation for all importers.

@reduz
Copy link
Member

reduz commented Jul 30, 2022

This would be good to salvage, but it likely got a bit more complicated with the new importer.

@akien-mga
Copy link
Member

I think this can be improved by properly overriding _get_property_list so no hacks are needed, I'll test.

@akien-mga akien-mga force-pushed the document-editor-import-options branch from 6694a9e to 65f84b9 Compare July 31, 2022 10:23
@akien-mga
Copy link
Member

Updated to improve the hack and make it work for all ResourceImporter-derived classes.

The old docs for ResourceImporterTexture are not included, they should be salvaged from https://github.com/godotengine/godot/blob/6694a9eba9b9b63436e44594b844101c56f7c74f/doc/classes/ResourceImporterTexture.xml for a follow-up PR (like for editor settings, I prefer we focus first on how to make those available for documentation, and the actual doc writing and proofreading should be done separately).

Comment on lines 10 to 11
<member name="_subresources" type="Dictionary" setter="" getter="" default="null">
</member>
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to check if this should be renamed or hidden.

Comment on lines 10 to 11
<member name="compress" type="bool" setter="" getter="" default="null">
</member>
Copy link
Member

Choose a reason for hiding this comment

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

Default values don't seem to be included properly, yet they're seen as "valid" so it shows default="null" for all (unlike e.g. EditorSettings where it doesn't show default).

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a second commit that solves this by calling get_import_options directly from DocTools so we can have access to the default value in ImportOption. It's a bit more hacky though. 09f0f70d81f50f7740268bcba4aff2106a9ec8cd

The less hacky option would be to properly register import options as properties in ClassDB with their default values, and then we don't need any of these hacks.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Aug 17, 2022
@YuriSizov
Copy link
Contributor

This would be nice to have, but seems to be in need of extra work. I'm kicking it to 4.x, but hoping that we can merge it in 4.0.

Tooltips are displayed when hovering import options, both in the Import
dock and in the import defaults editor (which is in the Project Settings).

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@Calinou Calinou force-pushed the document-editor-import-options branch from 09f0f70 to 8352122 Compare June 15, 2023 06:31
@Calinou
Copy link
Member Author

Calinou commented Jun 15, 2023

Rebased and tested again, it works as expected. Descriptions still need to be filled out, but the editor display works correctly.

Note that Advanced Import Settings options don't have a XML generated for yet. This should be tackled in a separate PR, as it's likely quite complex to get right.

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.

Looks good to me. We can merge after 4.1 is released and document all this for 4.2.

It's still a draft, anything major still pending?

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 15, 2023
@Calinou Calinou marked this pull request as ready for review June 15, 2023 07:01
@Calinou Calinou requested review from a team as code owners June 15, 2023 07:01
@Calinou Calinou requested review from a team as code owners June 15, 2023 07:01
@Calinou
Copy link
Member Author

Calinou commented Jun 15, 2023

It's still a draft, anything major still pending?

No, I forgot to mark it as ready for review. I just did that 🙂

@YuriSizov
Copy link
Contributor

Should we change the title and the commit message to something different? Objectively, this doesn't actually document much, but it allows for these options to be documented. We don't want to have misleading information in changelogs :)

@YuriSizov YuriSizov merged commit f25233c into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Uhm, I think I got distracted and merged it anyway :D Welp, no matter now. Thanks!

@Calinou
Copy link
Member Author

Calinou commented Jul 12, 2023

Uhm, I think I got distracted and merged it anyway :D Welp, no matter now. Thanks!

I'm starting work on documenting all import options – it shouldn't take too long before I open a PR with filled descriptions.

Edit: Done: #79405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add descriptions/tooltips to properties in the Import dock
4 participants