-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Correct PackedInt64Array comparison in description #99521
base: master
Are you sure you want to change the base?
Correct PackedInt64Array comparison in description #99521
Conversation
All Packed classes that have the same paragraph will compare the currently viewed Packed array type with the equivalent typed Array but here the comparison was with the Int32 version instead of the Int64 version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes sense - as noted, for the most part each Packed*Array
with a corresponding GDScript type has this note about differences, which directly compares the Packed*Array
with the equivalent typed array.
It does seem that PackedVector4Array
is missing such a note. I believe PackedVector4Array
is missing the note because the PR adding the PackedVector4Array
type (#85474) was added after the PR adding these notes (#78257). I'm not entirely sure why only some Packed*Array
types were documented in 78257 and some like PackedInt32Array
were left out.
I should also point out that these kind of subtly different copy-pasted notes are inherently a little problematic (there was already one followup copy-paste typo correction PR made: #97835). But that's a larger problem and not specific to these notes.
I saw the Vector4 one and forgot to add it. I'll add a commit for that too. |
…y PackedArray classes
I am unsure what should be done about the ones that don't have this paragraph because it inherently includes a comparison that we already agree has to be of the same type or risk confusing the reader. Perhaps it can be added to the ones that don't have it but omit the example but then one might then be left confused as to what the "equivalent" type of typed Array is. At the very least, even if they don't have this explanatory paragraph, they are also unlikely to confuse and the information does exist in the other classes and now in the main gdscript reference. So if someone wants to know more, they are likely to run into those. I'm guessing that if one assumes, in their head, that, for example, a PackedByteArray of the same length uses 4 times less memory than a PackedInt32Array and PackedInt32Array uses half as much as a PackedInt64Array, then they will be correct enough that it won't impact their decision of which one to use. edit - perhaps instead of these paragraphs, there should be a link to the gdscript reference section on PackedArrays? It seems a bit backwards though. For technical details you normally go to the class reference, not the tutorial section. Uncertain. |
Personally, I don't think that this PR needs to do anything more than the two changes you've done here, since now there is a consistent rule that @Mickeon is a lot more familiar with the standards for the class reference, so they will have some insight if they see this PR. edit: To your edit, in general we try to avoid linking from the class reference to the manual - the class reference is considered more of a source of truth since it's in the main repo, it's more likely to stay updated, and it should not have dependencies on the specific structure of the online manual. When we do link from the class reference to the manual, it's usually done in the tutorials section at the top of the class reference. |
That makes sense to me. I suppose this is for now a necessary evil (the copy-pastes) if the information is to be made more visible, since a user may simply look at one Packed*Array class in the reference and not all. The information provided is also, for at least part of the user-base, what they would assume anyway. So even if it's missing in some of the classes, it's not a big risk. Cleanest would be to have templates with placeholders but that's probably something that would already be in use if it was available. |
The PR is completely fine as is. Do not change it, it's okay for now. These... admittedly huge notes were added by @Calinou to mitigate confusion as much as possible. I personally loathe these kind of notes for many reasons. Hard to maintain, irritating to localize, exhausting to read, prone to become inconsistent. |
All Packed classes that have the same paragraph will compare the currently viewed Packed array type with the equivalent typed Array but here the comparison was with the Int32 version instead of the Int64 version.
PackedArrays that have a type not represented directly in base types do not have this paragraph but at least mention that if one wants the 64bit equivalent, they should look into the appropriate PackedArray.