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

Add Examples for export suffix hint feature in GDScript and C# #9785

Closed
wants to merge 1 commit into from
Closed

Add Examples for export suffix hint feature in GDScript and C# #9785

wants to merge 1 commit into from

Conversation

ShawnHardern
Copy link
Contributor

In response to the discussion in godot-proposals #10498, this pull request updates the documentation to surface this functionality

@ShawnHardern ShawnHardern changed the title Add Examples for Export Suffix Hint Feature in GDScript and C# Add Examples for export suffix hint feature in GDScript and C# Aug 19, 2024
Copy link
Contributor

@0stam 0stam left a comment

Choose a reason for hiding this comment

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

You should add dots at the ends of paragraphs for consistency with the rest of docs. Other than that looks great, I personally didn't know the unit hints exist ;)

According to the workflow you should also squash the commits before merging.

@ShawnHardern
Copy link
Contributor Author

ShawnHardern commented Aug 19, 2024

You should add dots at the ends of paragraphs for consistency with the rest of docs. Other than that looks great, I personally didn't know the unit hints exist ;)

According to the workflow you should also squash the commits before merging.

@0stam Good spot😄! thanks I have pushed an update, will squash the commits too

@tetrapod00
Copy link
Contributor

tetrapod00 commented Aug 19, 2024

You may also want to mention that vector types are supported. No need to list every type, but at least float and Vector3, and maybe also int would be good. Or maybe use "scalar and vector types" or "numeric types".

Quick testing on which types can be used with this property hint:

# Unsupoorted
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var unsupported_bool : bool
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var unsupported_color : Color
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var unsupported_string : String
# Supported
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_float : float
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_int : int
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec2 : Vector2
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec2I : Vector2i
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec3 : Vector3
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec3I : Vector3i
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec4 : Vector4
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_vec4I : Vector4i
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_quaternion : Quaternion
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_basis : Basis
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var supported_rect2 : Rect2
# Partially supported
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var partial_transform2d : Transform2D
@export_custom(PROPERTY_HINT_NONE, "suffix:m/s\u00b2") var partial_transform3d : Transform3D

Godot_v4 3-stable_mono_win64_iWBYinG6ku

Using any unit hint at all is nonsensical for some of these types - for example the transforms or quaternions. Possibly some of these behaviors should be changed on the engine side.

@ShawnHardern
Copy link
Contributor Author

@tetrapod00 Thank you for your awesome suggestions 👍🏻 and for taking the time to test the values! 🙌🏻. I've gone ahead and added your suggestions🎉.

@Piralein Piralein added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Aug 20, 2024
Co-authored-by: tetrapod <145553014+tetrapod00@users.noreply.github.com>

.. code-block:: csharp

[Export(PropertyHint.None,"suffix:m/s\u00b2")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, using \u00b2 for the squared symbol may merit a comment or a mention in the preceding sentence. Personally I think it's fine as is - curiosity is a good teacher.

Other than that, LGTM but my approval as just a contributor is not the one that matters for getting these changes merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @tetrapod00 thanks for the feedback!

I agree that it's probably a good idea to add a comment / clarification regarding the use of \u00b2, I'll wait for input from maintainers to see how they'd like to proceed.

@ShawnHardern
Copy link
Contributor Author

Think this PR will need some guidance from the maintainers regarding if the placement of the examples for export suffix hint in the page are alright.

Additionally some guidance in relation to @tetrapod00's comment would be helpful to determine if there are any missing explanations or additional examples should be included.

@Calinou
Copy link
Member

Calinou commented Nov 11, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:dotnet topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants