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 spy spider detection with detective scanner #1619

Merged
merged 16 commits into from
Dec 30, 2024

Conversation

Drsmail
Copy link

@Drsmail Drsmail commented Oct 25, 2024

Что этот PR делает

Фиксит баг с жучком, который не открепляется при скане.

Почему это хорошо для игры

Баги - это плохо.

Тестирование

https://github.com/user-attachments/assets/dbb862a8-a496-4197-a92b-66f5d7d39e02
Прикрепил жучок, просканировал вещь, получил жучок обратно в руку.

Changelog

🆑
fix: Шпионские жучки снова можно снимать, просканировав детективным сканером одежду, на которой он закреплён.
/:cl:

Summary by Sourcery

Allow removing spy bugs from clothing via detective scanner.

Bug Fixes:

  • Fixed a bug where spy bugs could not be removed after scanning the clothing they were attached to.

Enhancements:

  • Improved the interaction with spy bugs by allowing players to remove them directly after scanning.

@Drsmail Drsmail marked this pull request as ready for review October 25, 2024 22:22
@Drsmail
Copy link
Author

Drsmail commented Oct 25, 2024

Там очень странный if(!scanning) в /obj/item/detective_scanner/scan(atom/A, mob/user). Я тестил и с ним и без него. Код работает в обоях случаях, но вдруг я чего-то не понимаю и он нужен.

@Drsmail
Copy link
Author

Drsmail commented Oct 25, 2024

На будущее, куда тыкать, чтобы самому прикреплять ишью? Или права нужны?

@dj-34
Copy link
Collaborator

dj-34 commented Oct 25, 2024

На будущее, куда тыкать, чтобы самому прикреплять ишью? Или права нужны?

в теле PR'а писать Closes ссылка на пр

@m-dzianishchyts
Copy link
Collaborator

m-dzianishchyts commented Oct 25, 2024

На будущее, куда тыкать, чтобы самому прикреплять ишью? Или права нужны?

fix[es]/resolve[s]/close[s] #номер_ишуе в описании ПРа

modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
@dj-34
Copy link
Collaborator

dj-34 commented Oct 25, 2024

Название ПРа на английском и внятное название коммитов. Если фикс, то в коммите должно быть это упомянуто чтобы лейблер правильно срабатывал.

@dj-34 dj-34 changed the title Фикс для issue #1440 (Жучок не откреплялся при скане) Fix spy spider detection with detective scanner Oct 25, 2024
@dj-34 dj-34 added the 🔧 Фикс Переписываем ошибку так, чтобы она проявлялась в других обстоятельствах label Oct 25, 2024
@dj-34
Copy link
Collaborator

dj-34 commented Oct 25, 2024

Если есть возможность, запиши видео работы сканнера с жучком, прикрепи в ПР

@Drsmail
Copy link
Author

Drsmail commented Oct 26, 2024

Если есть возможность, запиши видео работы сканнера с жучком, прикрепи в ПР

Да, сейчас поправлю всё на что указали и запишу видео с тестом.

@github-actions github-actions bot removed the 🔧 Фикс Переписываем ошибку так, чтобы она проявлялась в других обстоятельствах label Oct 26, 2024
@Drsmail Drsmail requested a review from dj-34 October 26, 2024 04:30
@Drsmail
Copy link
Author

Drsmail commented Oct 26, 2024

Название ПРа на английском и внятное название коммитов. Если фикс, то в коммите должно быть это упомянуто чтобы лейблер правильно срабатывал.

Ну я вроде написал Fix, а бот всё равно убрал тэг, уууу глупая машина!

Copy link
Collaborator

@dj-34 dj-34 left a comment

Choose a reason for hiding this comment

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

Не лучшая реализация (точнее вообще не очень). Детективный анализатор сканирует на несколько тайлов, ты сканишь человека на расстоянии и находишь у него жучок который у тебя телепортируется в руку (а ты можешь даже не заметить).
Стоит сделать реализацию как это сделано с embed'ами (когда что-то острое вонзается в тело), то есть кликабельный текст "Найдено шпионское устройство!", при нажатии на которого через чек расстояния происходит снятие жучка в руку/на пол если рука занята.

@Drsmail
Copy link
Author

Drsmail commented Oct 28, 2024

Не лучшая реализация (точнее вообще не очень). Детективный анализатор сканирует на несколько тайлов, ты сканишь человека на расстоянии и находишь у него жучок который у тебя телепортируется в руку (а ты можешь даже не заметить). Стоит сделать реализацию как это сделано с embed'ами (когда что-то острое вонзается в тело), то есть кликабельный текст "Найдено шпионское устройство!", при нажатии на которого через чек расстояния происходит снятие жучка в руку/на пол если рука занята.

Спасибо за обратную связь! Я мог бы наверное закрыть это костылём, а именно:

  • Проблему с телепортом можно пофиксить, если добавить проверку на дистанцию.

Но думаю лучше будет сделать как ты предлагаешь, а заодно посмотреть про verb.

  1. Сканишь человека с любого расстояния.
  2. Вся одежда на нём проверяется (Теперь не нужно сканить именно одежду)
  3. Если найдён жучок, то получаешь кликабельный текст в чате. (Смотрю пример embed, делаю через verb)
  4. Нажимаешь на текст в чате, проходишь проверку по расстоянию, начинается процесс (Как сделать полоску зелёную над головой? Как этот модуль называется?) - получаешь жучок в руку.

Если такой вариант устроит, переделаю как будет время.

@dj-34
Copy link
Collaborator

dj-34 commented Oct 28, 2024

Не лучшая реализация (точнее вообще не очень). Детективный анализатор сканирует на несколько тайлов, ты сканишь человека на расстоянии и находишь у него жучок который у тебя телепортируется в руку (а ты можешь даже не заметить). Стоит сделать реализацию как это сделано с embed'ами (когда что-то острое вонзается в тело), то есть кликабельный текст "Найдено шпионское устройство!", при нажатии на которого через чек расстояния происходит снятие жучка в руку/на пол если рука занята.

Спасибо за обратную связь! Я мог бы наверное закрыть это костылём, а именно:

  • Проблему с телепортом можно пофиксить, если добавить проверку на дистанцию.

Но думаю лучше будет сделать как ты предлагаешь, а заодно посмотреть про verb.

  1. Сканишь человека с любого расстояния.
  2. Вся одежда на нём проверяется (Теперь не нужно сканить именно одежду)
  3. Если найдён жучок, то получаешь кликабельный текст в чате. (Смотрю пример embed, делаю через verb)
  4. Нажимаешь на текст в чате, проходишь проверку по расстоянию, начинается процесс (Как сделать полоску зелёную над головой? Как этот модуль называется?) - получаешь жучок в руку.

Если такой вариант устроит, переделаю как будет время.

Да. Не понимаю при чем тут верб, он тебе по сути не нужен. Касаемо полоски - do_after и do_after_once

Copy link

This pull request seems to be stale as there have been no changes in 14 days, please make changes within 7 days or the PR will be closed. If you believe this is a mistake, please inform a development team member on Discord.

@github-actions github-actions bot added the Stale ПР долго был не активен, и требует обновления. label Nov 12, 2024
@PhantornRU
Copy link
Collaborator

Если такой вариант устроит, переделаю как будет время.

Забросил?

@github-actions github-actions bot removed the Stale ПР долго был не активен, и требует обновления. label Nov 19, 2024
@Drsmail
Copy link
Author

Drsmail commented Nov 21, 2024

Если такой вариант устроит, переделаю как будет время.

Забросил?

Отложил в очень долгий ящик, но всё ещё в планах.

@Drsmail
Copy link
Author

Drsmail commented Nov 22, 2024

https://github.com/user-attachments/assets/d4a5387a-e72b-43d1-a920-8272fe56f991
Пока получилось сделать так. Что должно происходить при клике на человека? В старом коде и сейчас, жучок так не найти. @dj-34

image
Думаю ещё лучше сделать ссылку по другому, но пока не уверен, как её сделать правильно, чтобы хорошо смотрелось.

@dj-34
Copy link
Collaborator

dj-34 commented Nov 22, 2024

https://github.com/user-attachments/assets/d4a5387a-e72b-43d1-a920-8272fe56f991 Пока получилось сделать так. Что должно происходить при клике на человека? В старом коде и сейчас, жучок так не найти. @dj-34
image Думаю ещё лучше сделать ссылку по другому, но пока не уверен, как её сделать правильно, чтобы хорошо смотрелось.

Тебе не нужно изобретать велосипед, посмотри как сделаны впивающиеся предметы. Можешь прямо оттуда скопипастить код при осмотре.

Да, я посмотрел и украл это от туда. Что-то не так с реализацией?

Если так, то окей. Смутил отступ, положение и стилизация текста. Может я забыл как оно в оригинале выглядит.

@Drsmail
Copy link
Author

Drsmail commented Nov 22, 2024

https://github.com/user-attachments/assets/d4a5387a-e72b-43d1-a920-8272fe56f991 Пока получилось сделать так. Что должно происходить при клике на человека? В старом коде и сейчас, жучок так не найти. @dj-34
image Думаю ещё лучше сделать ссылку по другому, но пока не уверен, как её сделать правильно, чтобы хорошо смотрелось.

Тебе не нужно изобретать велосипед, посмотри как сделаны впивающиеся предметы. Можешь прямо оттуда скопипастить код при осмотре.

Да, я посмотрел и украл это от туда. Что-то не так с реализацией?

Если так, то окей. Смутил отступ, положение и стилизация текста. Может я забыл как оно в оригинале выглядит.

Мне тоже не очень стиль нравится, Отступ там через /t, могу его убрать, если надо? А вот как менять шрифт, цвет текста и т.д. я уже не знаю. Тогда просто убираю /t, исправляю очепятки и готово?

@Drsmail
Copy link
Author

Drsmail commented Nov 22, 2024

https://github.com/user-attachments/assets/e35c9475-ab3b-47f2-8a22-e95484e55ca1
Я понял, что ты имел ввиду. Сделал получше как мне кажется и код лишней убрал. + Всё протестировал @dj-34

@Drsmail Drsmail requested a review from dj-34 November 22, 2024 14:36
modular_ss220/spy_spider/code/spy_spider.dm Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
Copy link
Collaborator

@dj-34 dj-34 left a comment

Choose a reason for hiding this comment

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

Времени и желания смотреть код нет, по самой реализации - апрув (Если оно было сделано как обговорено).

@Drsmail
Copy link
Author

Drsmail commented Dec 15, 2024

@m-dzianishchyts, Максим, я понимаю это было давно, но вроде бы всё о чём мы говорили я сделал. Протестировал, у меня ничего не крашнулась, не дюпнулось. Можете на доверичах мержить.

@Drsmail
Copy link
Author

Drsmail commented Dec 16, 2024

@m-dzianishchyts Ну что мержим? notice я добавил.

@m-dzianishchyts
Copy link
Collaborator

@m-dzianishchyts Ну что мержим? notice я добавил.

Все еще используется sleep() вместо do_after() и не совсем там. Зачем резолвить обсуждение, если не сделано?

@Drsmail
Copy link
Author

Drsmail commented Dec 16, 2024

Все еще используется sleep() вместо do_after() и не совсем там. Зачем резолвить обсуждение, если не сделано?
Просто нигде же сканер, когда сканирует предмет, нету do_after. Я не понимаю, почему именно при скане одежды, должен происходить do_after() - этого не было изначально тут и это не то как сканер себя ведёт с другими предметами. А действие - которое требует времени - это снятие жучка.

Почему резольвнул? Потому что было у нас с тобой похоже обсуждение, в котором ты написал, что логика правильная, ну и подумал, что к этому этот комментарий, тоже относится.

Сделать мне не сложно, но тогда при сканирование любой одежды будет do_after? Может вообще стоит убрать sleep т.к. задержка через sleep и так есть в родительском методе? Как я понимаю тут основной посыл - сделать так, чтобы этими действиями не спамили?

@Drsmail
Copy link
Author

Drsmail commented Dec 30, 2024

@sourcery-ai review

Copy link

sourcery-ai bot commented Dec 30, 2024

Reviewer's Guide by Sourcery

This PR fixes a bug where spy spiders were not being detached when scanned by a detective scanner. The fix changes the way spy spider removal is handled. Instead of a verb, it now uses a topic link that is added when the detective scanner detects a spy spider. This topic link triggers the remove_spy_spider proc on the clothing item, which removes the spy spider and places it in the user's hand.

Sequence diagram for spy spider detection and removal process

sequenceDiagram
    actor User
    participant Scanner as Detective Scanner
    participant Clothing
    participant SpySpider

    User->>Scanner: Use scanner on clothing
    Scanner->>Clothing: scan()
    alt Spy spider found
        Scanner-->>User: Show 'Spy device found!' link
        User->>Clothing: Click removal link
        Clothing->>SpySpider: remove_spy_spider()
        alt Successfully put in hand
            SpySpider->>User: Move to user's hand
        else Hand full
            SpySpider->>Clothing: Drop to floor
        end
    else No spy spider
        Scanner-->>User: Show 'Nothing found' message
    end
Loading

Class diagram for spy spider and clothing interaction

classDiagram
    class Clothing {
        +obj/item/radio/spy_spider spy_spider_attached
        +remove_spy_spider(cloth_uid, spider_uid)
        +Topic(href, href_list)
    }

    class DetectiveScanner {
        -bool scanning
        +scan(atom/A, mob/user)
        +add_log(message)
    }

    class SpySpider {
        +forceMove(target)
    }

    Clothing "1" -- "0..1" SpySpider : has
    DetectiveScanner ..> Clothing : scans
    note for Clothing "Modified to use Topic-based
removal instead of verbs"
Loading

File-Level Changes

Change Details Files
Changed spy spider removal to use a topic link.
  • Removed the remove_spy_spider verb from the clothing item.
  • Added a Topic proc to the clothing item that handles the remove_spy_spider functionality.
  • Modified the detective scanner's scan proc to add a topic link to the chat message when a spy spider is detected.
  • Added a remove_spy_spider proc to the clothing item that takes the clothing and spy spider UIDs as arguments.
  • Updated the remove_spy_spider proc to handle the removal of the spy spider and placing it in the user's hand.
  • Added error handling and messages for various scenarios, such as the user being too far away, having their hands blocked, or the clothing not having a spy spider attached.
  • Updated grammar and declensions in several chat messages for better clarity and consistency.
modular_ss220/spy_spider/code/spy_spider.dm
Updated grammar and declensions in chat messages.
  • Corrected declensions in chat messages related to attaching and removing the spy spider.
  • Improved wording for clarity and consistency.
modular_ss220/spy_spider/code/spy_spider.dm

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Drsmail - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Drsmail
Copy link
Author

Drsmail commented Dec 30, 2024

@m-dzianishchyts Машина сказала, что я молодец! А если серьёзно, то всё готово.

modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
modular_ss220/spy_spider/code/spy_spider.dm Outdated Show resolved Hide resolved
Copy link
Collaborator

@m-dzianishchyts m-dzianishchyts left a comment

Choose a reason for hiding this comment

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

Валидити

@m-dzianishchyts m-dzianishchyts merged commit 9e859b9 into ss220club:master Dec 30, 2024
14 of 16 checks passed
@Drsmail Drsmail deleted the DetectiveSpySpiderFix branch December 30, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Жучок нельзя снять после сканирования одежды
5 participants