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

fix: allow elevator misalignment #3181

Merged

Conversation

scarf005
Copy link
Member

@scarf005 scarf005 commented Sep 19, 2023

Summary

SUMMARY: Bugfixes "Fix misaligned elevator not working due to misalignment"

Purpose of change

elevator misalignment was making elevator control not working.

Describe the solution

  1. aligned sarcophagus elevator.
  2. made elevator checks more forgiving (now considers up to 3-tile radius)
    3. inlined functions 'cause godbolt results told me they were more efficient

no inline (133 loc) vs inline (99 loc)

Tests

2023-09-19_22-56-01.mp4

@github-actions github-actions bot added JSON related to game datas in JSON format. src changes related to source code. labels Sep 19, 2023
@scarf005 scarf005 force-pushed the fix-sarcophagus-elevator-alignment branch from c1e270d to a75eba9 Compare September 19, 2023 14:10
@scarf005 scarf005 changed the title fix: elevator misalignment in sarcophagus fix: allow elevator misalignment Sep 19, 2023
@olanti-p
Copy link
Contributor

olanti-p commented Sep 19, 2023

inline is best suited for hot, or small and simple (like in your examples) functions where the call cost is noticeable relative to the work done. Using inline on large functions will inflate the size of the binary, which can actually worsen the performance on some CPUs, or it will simply do nothing because inline does not force the compiler to inline, it just advises it to, and the compiler is free to choose whether to inline the function or not regardless of whether the function has inline or not. It is hard to tell without a profiler whether you need to inline, and it can be hard to tell without disassembling the resulting binary whether the inline even worked.

In your 133 loc example, from what I can tell from the atrocious mobile UI, the compiler does inline the functions. It just does not eliminate functions themselves because they're not static, so the compiler thinks they can be used from other translation units, which would be impossible if they were eliminated.

TLDR: It is ok to have call overhead if you don't have a specific reason to avoid it.

@scarf005
Copy link
Member Author

scarf005 commented Sep 19, 2023

you're right, it's probably not worth it, as these functions aren't called in hot loop.

EDIT: C++ made same optimization (99loc) after adding anonymous namespace (== static linking).

@scarf005 scarf005 force-pushed the fix-sarcophagus-elevator-alignment branch from a75eba9 to 3c4d494 Compare September 19, 2023 16:21
@chaosvolt
Copy link
Member

../tests/translations_test.cpp:147: warning:
  Skipped (unable to set locale)

D:\a\_temp\bfbeaa38-6fcb-4d6b-806d-d1350dacae97: line 1:  1382 Segmentation fault      ./tests/cata_test.exe --rng-seed time

Unrelated I hope?

src/iexamine_elevator.cpp Outdated Show resolved Hide resolved
@scarf005 scarf005 force-pushed the fix-sarcophagus-elevator-alignment branch from 3c4d494 to 57fcb7f Compare September 23, 2023 05:26
@scarf005 scarf005 requested a review from olanti-p September 23, 2023 05:26
@scarf005
Copy link
Member Author

@chaosvolt i think this one is ready too

@chaosvolt chaosvolt merged commit 855e278 into cataclysmbnteam:upload Sep 24, 2023
14 of 18 checks passed
@scarf005 scarf005 deleted the fix-sarcophagus-elevator-alignment branch September 24, 2023 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants