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

Fixed segfault when an NPC completes a pickup after a player has alre… #72958

Conversation

Levitator0
Copy link
Contributor

@Levitator0 Levitator0 commented Apr 11, 2024

abusive off-topic commentary removed

…ady taken it

- Fixes CleverRaven#72340
- Replaces naked pointer npc::wanted_item with: item_location npc::wanted_item
- The safe reference will return null if the object gets deleted, which is what happens
  if someone else collects it before the NPC completes the pickup

TODO: Sensibly, we should track the item coordinates using item_location
since it's suppose to package a safe reference alongside location data.
However, npc::wanted_item_pos is subject to shuffling via the game::update_map()
and shift() mechanism, so rather than try to comprehend that and how it
does or doesn't interact with item_location,  I'm instead leaving
it alone since the coordinates appear to be calculated right at present.
@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership [C++] Changes (can be) made in C++. Previously named `Code` new contributor labels Apr 11, 2024
src/npcmove.cpp Outdated Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 2 commits April 10, 2024 22:42
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 2 commits April 10, 2024 22:54
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mqrause
Copy link
Contributor

mqrause commented Apr 11, 2024

Those suggested astyle changes strongly indicate you got some missing or extra braces.

Also please remove unnecessary code instead of just commenting it out. If you think a justification of the removal is necessary, put it in the PR text.

@Levitator0
Copy link
Contributor Author

Those suggested astyle changes strongly indicate you got some missing or extra braces.

Also please remove unnecessary code instead of just commenting it out. If you think a justification of the removal is necessary, put it in the PR text.

There was an ubraced single-line else statement, so maybe that's the issue. I don't think I know how to read some of those prompts, so I didn't commit them. If the devs want to assume style changes are always valid, they're welcome to apply them.

@mqrause
Copy link
Contributor

mqrause commented Apr 11, 2024

Please check the style guide for how we make sure everyone uses the same code style. Commits become very messy very fast if everyone uses their own favorite style.
As a rule of thumb, astyle is always right. And if code you didn't touch fails astyle checks, it's almost certainly an error in the code you did touch.

And as another rule of thumb, please don't mark suggestions as resolved when you're unsure about why they're there and what they try to achieve.

Edit: I left a comment on the third review, in case you didn't see that.

Levitator and others added 3 commits April 11, 2024 07:32
single-line else needing both braces

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 2 commits April 11, 2024 07:49
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Outdated Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 3 commits April 11, 2024 07:52
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 2 commits April 11, 2024 07:56
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/npcmove.cpp Outdated Show resolved Hide resolved
Levitator and others added 2 commits April 11, 2024 08:12
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Levitator0
Copy link
Contributor Author

Please check the style guide for how we make sure everyone uses the same code style. Commits become very messy very fast if everyone uses their own favorite style. As a rule of thumb, astyle is always right. And if code you didn't touch fails astyle checks, it's almost certainly an error in the code you did touch.

And as another rule of thumb, please don't mark suggestions as resolved when you're unsure about why they're there and what they try to achieve.

Edit: I left a comment on the third review, in case you didn't see that.

I'm beyond confused as to how this issue resolution system interacts with the lint bot. I have totally lost the thread. It seems like the number of issues to resolve has only climbed. It doesn't upfront distinguish between an issue that was closed accepted/committed or closed as disregarded. And now I have a bunch of style issues, where when I attempt to commit them, GitHub complains that edits on deleted lines are not supported. Well, then why is it proposing the commit? I have no idea how to use this arrangement. I'm going to try to lint the code locally and then either update the PR, or reissue it, because this is driving me crazy.

This is my first submission, and I ran into issues with the GitHub linter,
so hopefully this is way simpler.
@Levitator0
Copy link
Contributor Author

Ok, I linted locally, and I have to conclude that astyle flips out and becomes totally confused if you leave the braces off of a single-line else statement. In any case, the code passes the automated check on GitHub now, so I'm going to mark all of the linting conversations as resolved because linting passes both locally and online. Sorry for the mess, as this setup confused the daylights out of me.

@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Apr 11, 2024
@NetSysFire
Copy link
Member

  1. You can batch apply suggestions.
  2. You still need to fix your PR summary.

Also dear mergers, please squash this one.

@Levitator0
Copy link
Contributor Author

  1. You can batch apply suggestions.

    1. You still need to fix your PR summary.

Also dear mergers, please squash this one.

What's wrong with my summary?

@NetSysFire
Copy link
Member

The summary section must contain exactly one line, in the format of Category "Description here". See test failure of the PR validator test. The category to use here is Bugfixes. Move your closing keyword into the Purpose of change section.

If you stayed just 10 minutes longer on IRC I would have helped you there.

@github-actions github-actions bot added the <Bugfix> This is a fix for a bug (or closes open issue) label Apr 11, 2024
src/npcmove.cpp Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 11, 2024
Copy link
Member

@I-am-Erk I-am-Erk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my limited skill the code looks good, just some comments on comments.

src/npc.h Outdated Show resolved Hide resolved
src/npcmove.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 12, 2024
src/npcmove.cpp Outdated Show resolved Hide resolved
@Levitator0 Levitator0 closed this Apr 14, 2024
@Levitator0 Levitator0 deleted the fix-segfault-on-npc-stale-pickup branch April 14, 2024 07:54
@Zireael07
Copy link
Contributor

Why close?

@Levitator0

This comment was marked as abuse.

@Levitator0

This comment was marked as abuse.

@ZhilkinSerg
Copy link
Contributor

Why close?

I do not think they want to contribute.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions new contributor NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants