DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.88k stars 251 forks source link

Fix small collection of errors #548

Closed pzychotic closed 2 months ago

pzychotic commented 3 months ago

Pull Request Type

Description

Fixed a bunch of small errors, like inverted error checks that do nothing, assignments in 'if' statements and missing va_end calls.

Related Issues

Screenshots (if applicable)

Checklist

Additional Comments

winterheart commented 2 months ago

That interesting things... I wonders that some of these bugs actually now "features" that we should reproduce as broken logic for playing levels...

pzychotic commented 2 months ago

That interesting things... I wonders that some of these bugs actually now "features" that we should reproduce as broken logic for playing levels...

If it breaks gameplay logic that is expected to work like this, then I might agree. But I haven't seen anything going wrong with these changes yet (in my limit testing to be fair).

pzychotic commented 2 months ago

The source code is decidedly UTF-8 (as indicated by CMakeLists.txt), so the use of U+2019 does not constitute a bug per se.

I actually get a MSVC compiler warning without that change: Warning C4828 The file contains a character starting at offset 0x7241 that is illegal in the current source character set (codepage 65001). Notepad++ shows this file to be ANSI, when saving as UTF-8 I get a change to that character and the warning is fixed.

jengelh commented 2 months ago

The source code is decidedly UTF-8 (as indicated by CMakeLists.txt), so the use of U+2019 does not constitute a bug per se.

I actually get a MSVC compiler warning without that change: Warning C4828 The file contains a character starting at offset 0x7241 that is illegal in the current source character set (codepage 65001).

I looked again, and you are right. It is actually is a lone 0x92 byte, but the Github webinterface just silently upconverted it on a heuristic basis, fooling everyone :-/ (cp1252:0x92 is U+2019 indeed)

Lgt2x commented 2 months ago

even though it's technically changing logic, most changes seem rather innocuous. The only one I'm not sure about is the one in physics.cpp : some mechanics could have been applied to other types of objects not intended because the OBJ_CLUTTER test succeeded all the time. Do you think that it's reasonable to merge as it is @winterheart ? I don't think it will cause big problems anyway

winterheart commented 2 months ago

I'm concerning that would change logic in level. Need to test these changes.

jengelh commented 2 months ago

I have given this a look, by replacing the lone OBJ_CLUTTER by a function XOBJ_CLUTTER(obj):

bool XOBJ_CLUTTER(const object *obj) {
    if (obj->type == OBJ_CLUTTER)
        return true;
    printf("otype %u\n", obj->type);
    return false;
}

The path to these places suggested to me that a clutter objects needs to collide with terrain, but even on level 3 (lightning tower balls) that did not seem to readily trigger.

pzychotic commented 2 months ago

I have given this a look, by replacing the lone OBJ_CLUTTER by a function XOBJ_CLUTTER(obj): ... The path to these places suggested to me that a clutter objects needs to collide with terrain, but even on level 3 (lightning tower balls) that did not seem to readily trigger.

Nice approach!

If that change is too worrisome, I can split it out into a separate PR. Just didn't want to create a bunch of PRs for these little changes.

jengelh commented 2 months ago

Yeah the last two commits seem to warrant more investigation in their own right. Btw, commit summaries should not use a trailing period.

pzychotic commented 2 months ago

Updated without the possibly game play altering fixes.