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

PDA messaging Port #245

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Conversation

Ermucat
Copy link

@Ermucat Ermucat commented Dec 25, 2024

About the PR

Added the new app Nanochat, allowing crewmembers to chat if given their nanochat ID
Port of DeltaV-Station/Delta-v#2362 so props to them

Why / Balance

This gives traitors a new way of exchanging information instead of just sitting a corner, adds depth, and potential to be utilized

Technical details

Changed LogProbe UI and system
Changed Agent ID UI and system
Added DeltaV folders for Content.Server,Content.Client,Content.Shared
Added new nanochat ui
Added new nanochat system
added new nanochat components

Media

2024-12-25.12-35-23.mp4

Requirements

Breaking changes

Changelog

🆑

  • add: Added a new program to your pda, nanochat!

Copy link

github-actions bot commented Dec 25, 2024

RSI Diff Bot; head commit c7485fc merging into 5ddc3ec
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/_DeltaV/Misc/program_icons.rsi

State Old New Status
nanochat Added

Resources/Textures/_DeltaV/Objects/Devices/cartridge.rsi

State Old New Status
cart-chat Added

Edit: diff updated after c7485fc

Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

After first quick glance:

@FluffMe FluffMe added S: Awaiting Changes Reviewer requested changes and removed S: Needs Review Review is requested labels Dec 25, 2024
@FluffMe
Copy link
Collaborator

FluffMe commented Dec 25, 2024

FYI, you can run the tests locally, not on github. So that you can fix everything before you push and be mostly sure that it's fixed.

The command to run the test is listed in the test: https://github.com/ss14-harmony/space-station-14/actions/runs/12495369896/job/34869788588#step:10:3 on the example of Integration tests.

Currently even Build fails, so please test changes before pushing them.

@Ermucat Ermucat requested a review from FluffMe December 25, 2024 23:53
@github-actions github-actions bot added S: Needs Review Review is requested and removed S: Awaiting Changes Reviewer requested changes labels Dec 25, 2024
This reverts commit ea7cc2e.
Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

Not sure why the review popped again, but:

  • Linter fails
  • Build fails
  • Tests fail

It got worse. Please fix before requesting review.

I am drafting this. Please undraft when it's in ready and test passing state.

@FluffMe FluffMe added DO NOT MERGE and removed S: Needs Review Review is requested labels Dec 25, 2024
@FluffMe FluffMe marked this pull request as draft December 25, 2024 23:59
@Ermucat Ermucat marked this pull request as ready for review December 26, 2024 00:03
This reverts commit e1732c4.
@FluffMe
Copy link
Collaborator

FluffMe commented Dec 26, 2024

Why is this undrafted?

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 26, 2024

Same test fail. Please don't undraft until you fix this.
Run tests locally with

dotnet test --no-build --configuration DebugOpt Content.IntegrationTests/Content.IntegrationTests.csproj -- NUnit.ConsoleOut=0 NUnit.MapWarningTo=Failed

@FluffMe FluffMe marked this pull request as draft December 26, 2024 00:39
@Ermucat
Copy link
Author

Ermucat commented Dec 26, 2024

It is claiming that it cant find Content.Integratiotests/Content.Intergrationtests.dll when I run the command

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 26, 2024

It is claiming that it cant find Content.Integratiotests/Content.Intergrationtests.dll when I run the command

  • You must build the project first (launching the dev build should include building). Or remove --no-build flag to build.
  • Command should be executed in the folder of the repo with the branch checked out.

@Ermucat
Copy link
Author

Ermucat commented Dec 26, 2024

image

I think something has gone very wrong. what do I do?

Okay so Ive reverted to the orignal commit "It is finished" this has fixed the issue, now its just res.typecheck: Sandbox violation: Access to type not allowed: [Content.Shared]AgentIDCardJobIconChangedMessage

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 26, 2024

image

I think something has gone very wrong. what do I do?

Okay so Ive reverted to the orignal commit "It is finished" this has fixed the issue, now its just res.typecheck: Sandbox violation: Access to type not allowed: [Content.Shared]AgentIDCardJobIconChangedMessage

In Content.Shared/Access/SharedAgentIDCardSystem.cs (https://github.com/ss14-harmony/space-station-14/blob/b7b33590368bd12bb403830e369dd82cd77682b0/Content.Shared/Access/SharedAgentIDCardSystem.cs) you define the following on lines 87-119:

  • AgentIDCardNameChangedMessage
  • AgentIDCardJobChangedMessage
  • AgentIDCardJobIconChangedMessage

This file already has defined:

All three of them are sealed, and trying to redeclare them might trigger the error you are seeing.
All three of them are identical to the ones you have added. So I have no idea why you did that, and you should remove it regardless of error or not.

  • Try it. See if tests pass.
  • Go over Milon's PR and make sure you are adding only the things that need to be added.

@Ermucat
Copy link
Author

Ermucat commented Dec 26, 2024

I will test this soon, thank you so much.

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 26, 2024

If this doesn't help, dig deeper into anything that touches AgentIDCardJobIconChangedMessage in your code and compare it carefully to Milon's implementation. Might be public keyword missing somewhere or something else that is minor but that trips the type checker.

But probably this is it.

@Ermucat Ermucat marked this pull request as ready for review December 26, 2024 05:39
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.

3 participants