Cleanup z_collision_check 1

[Dragorn421]

No description provided.

[mzxrules]

Suggested change

	/* 0x00 */ ColliderElement base;
	/* 0x00 */ ColliderElement elem;

not sure why you would want this to be inconsistent with e.g. ColliderCylinder

[Dragorn421]

The parent of ColliderElement for JntSph and other multi-elements colliders, is already an element, so this would mean jntSphElem->elem instead of the more natural (?) jntSphElem->base

and cylinders and other single-element colliders ofc already have base for the collider, and they don't have a specific element substruct, so their (basic) element is just "elem"

I could change the single-element colliders' elements to be specific structs, but it would just be bloat for no reason imo

[mzxrules]

reason for calling Collider base in e.g. JntSph struct is because Colliders utilize a bit of polymorphism (e.g. SetAT accepts Collider* and downcasts to ColliderJntSph*). I don't think this is the case for ColliderElement. Additionally, I'd say it's a bit weird to only have some ColliderElement consumers to express polymorphism but not others.

[Dragorn421]

ColliderElement is at the start of every Collider*Element, and that naming pattern seems to justify the inheritance pattern (note the naming comes with the PR, ColliderElement was ColliderInfo-something)

See also #1427 (comment) for why I think it makes sense to consider cylinders and such as single-elements colliders

and again

I could change the single-element colliders' elements to be specific structs, but it would just be bloat for no reason imo

[mzxrules]

but then you also continued saying

it would just be bloat for no reason imo

My position is you either implement ColliderShapeElements for all shapes, or you do it for none and change base ->elem. There's no reason to go half and half here. Personally, I don't think the inheritance pattern is strictly necessary, which is why I didn't use it when decomping this code.

[Dragorn421]

I think it's a bad idea to make a change that would turn

static ColliderCylinderInit sCylinderInit = {
    {
        COLTYPE_NONE,
        AT_NONE,
        AC_ON | AC_TYPE_ALL,
        OC1_ON | OC1_TYPE_ALL,
        OC2_TYPE_2,
        COLSHAPE_CYLINDER,
    },
    {
        ELEMTYPE_UNK2,
        { 0x00000000, 0x00, 0x00 },
        { 0xFFCFFFFF, 0x00, 0x00 },
        TOUCH_NONE,
        BUMP_ON,
        OCELEM_ON,
    },
    { 25, 60, 0, { 0, 0, 0 } },
};

into

static ColliderCylinderInit sCylinderInit = {
    {
        COLTYPE_NONE,
        AT_NONE,
        AC_ON | AC_TYPE_ALL,
        OC1_ON | OC1_TYPE_ALL,
        OC2_TYPE_2,
        COLSHAPE_CYLINDER,
    },
    {
        {
            ELEMTYPE_UNK2,
            { 0x00000000, 0x00, 0x00 },
            { 0xFFCFFFFF, 0x00, 0x00 },
            TOUCH_NONE,
            BUMP_ON,
            OCELEM_ON,
        },
        { 25, 60, 0, { 0, 0, 0 } },
    },
};

or (if one comma is omitted):

static ColliderCylinderInit sCylinderInit = {
    {
        COLTYPE_NONE,
        AT_NONE,
        AC_ON | AC_TYPE_ALL,
        OC1_ON | OC1_TYPE_ALL,
        OC2_TYPE_2,
        COLSHAPE_CYLINDER,
    },
    { {
          ELEMTYPE_UNK2,
          { 0x00000000, 0x00, 0x00 },
          { 0xFFCFFFFF, 0x00, 0x00 },
          TOUCH_NONE,
          BUMP_ON,
          OCELEM_ON,
      },
      { 25, 60, 0, { 0, 0, 0 } } },
};

(note these are the Init structs but I assume we want to keep them mirroring the live structs)

even though it would fit the pattern of elements being ColliderElement base; followed by element dimensions

Or do you really think it's worth it

[mzxrules]

well... that's the price you pay for consistency. I do hate it makes var access a little longer, but you end up with this:

cylinder->element.base
cylinder->element.dim
jntSphr->elements[0].base
jntSphr->elements[0].dim

and i wouldn't drop the comma personally. if you do go through with it, I would check that cylinder OK's first, as I did it locally on quads and collision_check.c ok'ed fine (didn't compile other actors tho)

[Dragorn421]

Sticking to ColliderJntSph.elements[].base and ColliderCylinder.elem, at least for now.

[Dragorn421]

I would like to have feedback on this change:
cfc8c32
(introducing a new struct ColliderCylinderElement)

before I update all the actors. That commit only changes three actors, as a demonstration

[AngheloAlf]

Personally I like it, I think it looks better and makes more sense this way

[hensldm]

Changes seem good to me :) .

[fig02]

Ive made a pull request to this branch with some organizational changes id like to see in the colcheck header: Dragorn421#3

Mainly:

• I think it makes sense for cylinders to be the first "shape" in the header, instead of JntSphere, simply because of how common cylinders are in comparison to anything else
• Within each collider "shape", I separated the "live" collider structs from the init data. It helps me personally to see all of the nested structs next to each other, and have them be separated by their purpose.
• I added a brief description of each shape, mainly so that there is a reference of what "jntsph" even means. And it helps quickly explain which are a "single" entity and which are collections

[fig02]

I reverted the change to make Cyl come first, mzx brought up a good point in the PR that the current order respects the enum order. I didnt consider that.

[hensldm]

Changes still seem good to me :)

[hensldm]

Probably not in this PR, but thoughts on shortening colType to just type. Seems weird type gets the qualifier, but shape doesn't

[mzxrules]

strong disagree. If any property in Collider were to be called type, shape would be it, since the shape of a collider is it's most distinguishing feature; calling colType simply type would arguably be confusing and misleading.

The way I see it, colType/ColliderType are just placeholder names until we have a more complete understanding of the ColliderType enum's purpose, and so the name is fine as is.

[Dragorn421]

yeah this field isn't as important as "type" may make it sound

an appropriate name may or may not be material or texture

[fig02]

Merging sometime during the day on wednesday, last call for reviews