godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.3k stars 21.24k forks source link

Correct PackedInt64Array comparison in description #99521

Open mechalynx opened 10 hours ago

mechalynx commented 10 hours ago

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.

mechalynx commented 9 hours ago

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

mechalynx commented 9 hours ago

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 commented 9 hours ago

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 commented 8 hours ago

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 commented 2 hours ago

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.