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

Smart lamp causes segfault when plugged in and turned on with 0 battery #76319

Open
CoroNaut opened this issue Sep 10, 2024 · 3 comments
Open
Labels
(P3 - Medium) Medium (normal) priority (S2 - Confirmed) Bug that's been confirmed to exist

Comments

@CoroNaut
Copy link

Describe the bug

The smart lamp causes a segfault when plugged in and turned on with 0 power. If the smart lamp has charges in its internal battery regardless of it being plugged in or not, no segfault.

#76261 fixed #76245. Unfortunately, this problem was concealed by that one. it looks like "item::getlight_emit()" is the culprit this time.

Attach save file

TESTINGTHREE-trimmed.tar.gz

Steps to reproduce

  1. Spawn storage battery and smart lamp
  2. Turn on smart lamp to get its battery to 0
  3. Plug smart lamp into storage battery
  4. Turning it on causes segfault

Expected behavior

No segfault

Screenshots

No response

Versions and configuration

  • OS: Windows
    • OS Version: 10.0.19045.4651 (22H2)
  • Game Version: cdda-experimental-2024-09-10-0339 c73e4c6 [64-bit]
  • Graphics Version: Tiles
  • Game Language: System language []
  • Mods loaded: [
    Dark Days Ahead [dda],
    Disable NPC Needs [no_npc_food],
    Portal Storms Ignore NPCs [personal_portal_storms],
    Slowdown Fungal Growth [no_fungal_growth]
    ]

Additional context

crash.log

@CoroNaut CoroNaut added the (S1 - Need confirmation) Report waiting on confirmation of reproducibility label Sep 10, 2024
@PatrikLundell
Copy link
Contributor

PatrikLundell commented Sep 10, 2024

/Confirmed

although I would have preferred if the save had depleted the battery to make it easier to verify... I had some issues trying to actually get rid of the charges (ended up turning on/off repeatedly to shave one charge off each activation).

Edit:
To debug it, I had to compile it with debug mode, and to be able to use a breakpoint I had to turn the smart lamp off after draining it, then plug it into the network, place breakpoint, and then turn the lamp on. Not turning the lamp off causes constant breakpoint triggers, probably due to display frame calls.

Blows up at item.cpp operation item::getlight_emit() line
const ammotype &loaded_ammo = ammo_data()->ammo->type;
item.cpp operation item::ammo_data line
return !contents.empty() ? contents.first_ammo().ammo_data() : nullptr; gets called, which returns a null pointer as the contents of the "magazine" is missing.

item::getlight_emit checks whether "ammo" is absent, but does so including linked "ammo", i.e. from the linked battery. The following code then proceeds to use the item's internal "ammo" exclusively, blowing up when it's absent.

There are (at least) two ways to deal with this:
A. Change the if( ammo_remaining( true ) == 0 ) { line to pass "false" into the call to ignore the linked battery. That would get around the blowing up thing, but would also mean you'd need a charged battery to get the item to work even when plugged in.
B. Change the code for dimming the light when the battery runs out to:

  1. Handle the absence of access to local "ammo" (i.e. not blow up when the internal "ammo" is absent).
    2a. Ignore dimming effects if the item is plugged in and has sufficient juice (internal + external); or
    2b. Find another way to get at the capacity of the empty battery and use that to apply dimming when the combined internal and external juice available is less than 20% of the internal battery capacity.
    I don't feel qualified to make the required decisions.

@github-actions github-actions bot added (S2 - Confirmed) Bug that's been confirmed to exist and removed (S1 - Need confirmation) Report waiting on confirmation of reproducibility labels Sep 10, 2024
@CoroNaut
Copy link
Author

CoroNaut commented Sep 10, 2024

In your A solution, it is definitely reasonable to only allow the light when it has a charge. Think of a laptop, for example. The laptop uses charge, and when plugged in, it simply charges the battery.

Dimming the light would be nice, but it's a smart lamp. It could be smart lamp lore to turn off at any hint that the battery couldn't support it, so it wouldn't damage itself.

@GuardianDll
Copy link
Member

Second of Patrik suggestion would be the best one

@GuardianDll GuardianDll added the (P3 - Medium) Medium (normal) priority label Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(P3 - Medium) Medium (normal) priority (S2 - Confirmed) Bug that's been confirmed to exist
Projects
None yet
Development

No branches or pull requests

3 participants