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

Correct PackedInt64Array comparison in description #99521

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mechalynx
Copy link

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.

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
@mechalynx mechalynx requested a review from a team as a code owner November 22, 2024 04:25
Copy link
Contributor

@tetrapod00 tetrapod00 left a 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.

@tetrapod00 tetrapod00 added documentation bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 22, 2024
@tetrapod00 tetrapod00 added this to the 4.4 milestone Nov 22, 2024
@mechalynx
Copy link
Author

I saw the Vector4 one and forgot to add it. I'll add a commit for that too.

@mechalynx
Copy link
Author

mechalynx commented Nov 22, 2024

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.

@tetrapod00
Copy link
Contributor

tetrapod00 commented Nov 22, 2024

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 Packed*Array types with an exact corresponding GDScript type have this note, and the others do not.

@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.

@mechalynx
Copy link
Author

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.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 22, 2024

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.
There are ideas thrown around to implement "macros" of some kind, which would copy and paste repetitive information wherever it is desired. Nothing much discussed about it though.

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

Successfully merging this pull request may close these issues.

3 participants