GitExl / WhackEd4

As a replacement for the DOS-based Dehacked, WhackEd4 allows you to load and edit Doom Dehacked files.
http://www.teamhellspawn.com/exl/whacked4
BSD 2-Clause "Simplified" License
41 stars 7 forks source link

Thing editor suggests floating point numbers for Thing properties that can't be that. #36

Open Acts19quiz opened 1 year ago

Acts19quiz commented 1 year ago

Version 1.2.3 through 1.3.0 beta 2

OS Windows 10 64-bit LTSC

Installed Python version 3.11.1 64-bit

Description Beginning in 1.2.3, WhackEd4 suggests floating point numbers for Thing properties (Radius and Height) that don't accept such numbers. This creates a few different issues:

Cause Like with #35, the cause isn't tied to a specific commit. Rather, it's when the switch was made from Python 2.7 to 3.7. Regardless of wxPython version (whether 2.7-era 4.0.0b or 3.7-era 4.0.6), building a commit like ac38c2 with 3.7 causes the issue, while 2.7 doesn't have it.

Potential suggestion Besides fixing it to not suggest floating point numbers for properties that do not support it, refusing to accept "." and other nonsense strokes in relevant fields (like Height and Radius) would both 'dummy proof' it, as well as act as a safety net in case a future Python update causes another regression.

GitExl commented 2 months ago

Either I don't understand the problem or I cannot reproduce it anymore. Entering 8.0 for a fixed point value like height will output 8*65536 in the patch (I'm not doing that math!), writing just 8 will also write the proper fixed point value in the patch.

Does this still happen with the latest version (beta 3)?

Acts19quiz commented 2 months ago

The first issue is that nothing other than a simple integer should be displayed at all for Radius and Height in the Thing Editor, since to my knowledge, no Doom source port in existence accepts fixed or floating point numbers for Radius and Height.

Reproduction of this error in Beta3):

  1. Launch WhackEd4
  2. Open the "Things" editor
  3. Open the "Properties" tab
  4. Observe that floating point numbers are now displayed for Radius and Height (formerly shown as integers in prior versions of WhackEd4, which is correct behaviour)

The issues go a little deeper when it comes to saving changes to patches.

I should note that 1.2.3 is the last stable version where the actual decimal points get saved to files, so the current beta(s) being cited for that specific issue is an error on my part.

However, that isn't quite the end.

  1. As before, open the "Things" "Properties" tab.
  2. Input a number into Radius or Height that has decimal points, any number of them. Try 35.4, for example.
  3. You'll now get a patch with a value (multiplied by 65536) that... isn't a whole number when divided by 65536.

I don't know how various source ports handle this situation. Maybe they round down or up to a whole number, or maybe they do actually try to create an object with a fractional Radius or Height.

My question is: Is there a source port in existence that supports fractional Radius and Height, establishing an actual use case for WhackEd4 allow it as input (as WhackEd does now in its current bets)? I'm actually curious to know, as the subject is somewhat interesting to me (how various source ports handle odd cases like fractional Radius and Height, and if they actually create such objects properly).

GitExl commented 2 months ago

Internally, Doom stores these fields as fixed point and Dehacked was built to literally overwrite these in the executable. DSDADoom will simply put the height or radius values into the thing data, having the same effect. No conversion at all. So it makes sense that it could in theory use fractional units there. GZDoom also converts it to a float by doing / 65535.

To test that theory, I did a quick test with DSDADoom. Made a patch that changes the player height to 56.1. With that patch loaded you cannot pass through a 56 unit tall sector. But without the patch, you can. And a player with a 56.9 unit height can pass through a 57 unit tall sector. This obviously isn't actually useful because sector heights are integers. But at least DSDADoom does interpret the values in the patch as one would expect, as fixed point.

Acts19quiz commented 2 months ago

I see. Thanks for the technical explanation. I wasn't aware of this.

Out of curiosity, is the same true of Radius? I suspect it is based on what's been mentioned so far.

In any event, the old bug of ".0" being added at the end is no longer present, so if the same is true of Radius as it is of Height, then this can be effectively discarded.