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 further details on properties returning Packed*Array #85207

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

AThousandShips
Copy link
Member

Limited it to properties with setters and existing documentation, might be excessive but without some central point to point to explaining this I feel this is a relevant point to clarify generally (also easier to access the information from the built-in documentation)

Left the documentation for Gradient as is as it has more specific details, could possibly add specific details to some of these if they have specific setters for details, but left it simple for now

@AThousandShips AThousandShips added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 22, 2023
@AThousandShips AThousandShips force-pushed the array_doc branch 2 times, most recently from 4d8f6c2 to 980dc72 Compare January 10, 2024 11:20
@Mickeon
Copy link
Contributor

Mickeon commented Jan 16, 2024

I don't know how to feel about this. The note of this PR talks about "returned values" and "methods", but the subject is always a property. The meaning of the note can only be inferred if you know what setters and getters are, and is otherwise still a bit puzzling.

Also consider that both GDScript and C# discourage using setters/getters for built-in classes whenever possible, in favour of properties.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 16, 2024

This is an existing note applied to more places, I didn't write this just updated one and copied it around, this was done in (and has been in there for almost 5 years):

@Mickeon
Copy link
Contributor

Mickeon commented Jan 16, 2024

I'm aware it does exist elsewhere, I am just a not fan of the way it is worded when applied to an Object's properties.

@YuriSizov
Copy link
Contributor

I mean I get the issue, but adding this note to absolutely every case of using packed arrays in the entire Godot API seems like an overkill.

@AThousandShips
Copy link
Member Author

Can work on the verbosity, or add a note to the packed arrays themselves and link to it instead, but this thing has caused a lot of confusion and I think just putting it in the manual isn't enough (and it isn't there yet either)

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 23, 2024

Yeah, I am familiar with the kind of confusion it can cause, been there 🙂 But from the maintenance perspective this is unrealistic to keep up and keep in sync. Should we work on a more general solution if a method returns a value vs a reference? That way we could automate the notes/visual indication in the docs.

Edit: Although here it's members, and not methods, which makes it even worse. I dunno, maybe this is the only way.

@AThousandShips
Copy link
Member Author

Could have an automatic addition, can look into that, but I think important to have self contained in the built in docs

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 23, 2024

Thinking an addition to all the Packed*Array classes on the lines of:
Note: Even though Packed*Array is shared like Array (any modifications to one changes all copies, unless duplicated with duplicate), a Packed*Array returned from a property or from built-in methods it is copied from the original, and modifying the returned array will not modify the property, instead make your modifications to the returned array and then assign it to the property again.

Or something, will see the wording in Array, and then maybe an automatic addition on properties returning a packed array type, that might be intrusive but will see, for translation it should be in the file id say

Perhaps an automatic message of:
Note: The returned Packed*Array is not shared and updates to it will not update this property, see Packed*Array for more details.

@AThousandShips AThousandShips force-pushed the array_doc branch 2 times, most recently from 3a32a97 to c97b6ae Compare January 23, 2024 15:39
@AThousandShips
Copy link
Member Author

Added notes to Packed*Array and will look at adding automatic details to properties next

@AThousandShips
Copy link
Member Author

AThousandShips commented Jan 23, 2024

Added an automatic note in the built-in documentation, will see how to implement the same for the rst generation etc., if this is the way to go

Will also remove notes that already exist

Any suggestions on either wording is welcome

@@ -1831,6 +1831,15 @@ void EditorHelp::_update_doc() {
class_desc->push_indent(1);
if (!cd.properties[i].description.strip_edges().is_empty()) {
_add_text(DTR(cd.properties[i].description));
// Add copy note to built-in properties returning Packed*Array.
if (!cd.is_script_doc &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Restricting to built-in classes at the moment, as from what I can tell at least for GDScript the data isn't copied like this but remains shared, but will investigate further as well

@AThousandShips
Copy link
Member Author

Not extremely familiar with the doc generation system but attempted to add the same feature to the rst side, any hints would be very welcome

@AThousandShips AThousandShips removed the request for review from a team January 23, 2024 17:57
@AThousandShips AThousandShips force-pushed the array_doc branch 2 times, most recently from 06b120a to c5a1b76 Compare February 17, 2024 18:57
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Looks great and has been needed for a while.

@AThousandShips
Copy link
Member Author

AThousandShips commented Feb 17, 2024

Someone well versed in the rst generation and style would be appreciated to take a look at that side, it looked like it works correctly but I haven't worked with that side properly

CC @Calinou I believe you've done some work in this area

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 29b3d9e), it works as expected.

image

editor/editor_help.cpp Outdated Show resolved Hide resolved
@mhilbrunner
Copy link
Member

Commits need to be squashed, besides that and the small note above this LGTM.

@AThousandShips
Copy link
Member Author

Good, had separate commits for specific things, will fixup and squash

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 6, 2024
@akien-mga
Copy link
Member

Needs rebase.

@akien-mga akien-mga merged commit 10d48d3 into godotengine:master Apr 8, 2024
16 checks passed
@AThousandShips AThousandShips deleted the array_doc branch April 8, 2024 12:11
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member Author

Thank you! Great to get this merged, learned entirely new areas of the system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release documentation enhancement topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Direct manipulation of Polygon2D polygon array elements possible in GDScript but not in CSharp.
6 participants