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

Trade crate storage machine for cargo ships #3016

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

Conversation

GreaseMonk
Copy link
Contributor

@GreaseMonk GreaseMonk commented Mar 2, 2025

About the PR

This PR adds a machine and a rack that allows storage of trade crates on cargo ships.
It is intended for mappers to allow adding these to the ships.
Conveyor belts can be constructed under it for automation! Heck yes!
Any crate that has the trade crate component is insertable and there is no whitelist for this.

Once they are grabbed by the machine, they are moved to a frozen map until they are called back again.

TODO:

  • Construction graphs
  • Destruction comps
  • Animation on racks

Why / Balance

This is a way to get rid of the crate stacking mechanics currently in place.

How to test

Open entity spawn panel and look for "cratestorage".
Spawn a crate storage machine and a few racks.
Link a button or a remote signaller to the crate storage machine by right clicking the machine, then clicking on the machine icon in the pop-up list.
Any crate that has a trade crate component will work.

Media

cratestoragerack_2.mp4

Requirements

Breaking changes

Changelog

🆑

  • add: Added crate storage machine and rack for trade crate storage on ship.

Copy link
Contributor

github-actions bot commented Mar 2, 2025

RSI Diff Bot; head commit 55e6d36 merging into 15c48c5
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/_NF/Structures/Machines/crate_machine.rsi

State Old New Status
closing Modified
open Modified
opening Modified
crate Removed

Resources/Textures/_NF/Structures/Storage/crate_storage_rack.rsi

State Old New Status
base Added
bottom Added
top Added

Edit: diff updated after 55e6d36

@GreaseMonk
Copy link
Contributor Author

This PR needs a quick glance over review before doing a more thorough review. There's still plenty to improve i am sure

@Houtblokje
Copy link
Contributor

How much does each rack hold?

@GreaseMonk
Copy link
Contributor Author

How much does each rack hold?

It currently holds two each. Let me know if you think it needs more, ill rework sprites

Copy link
Contributor

github-actions bot commented Mar 3, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the S: Merge Conflict This PR has conflicts that prevent merging label Mar 3, 2025
@dustylens
Copy link
Contributor

Given the infrastructure making it hold three crates per tile would be a pretty distinct upgrade over simply attempting to smash crates via conveyor.

@GreaseMonk
Copy link
Contributor Author

GreaseMonk commented Mar 4, 2025

Given the infrastructure making it hold three crates per tile would be a pretty distinct upgrade over simply attempting to smash crates via conveyor.

i figured myself though that the benefit of it being automatable with conveyors already is huge
but i'll poll around! thank you for the feedback

@GreaseMonk
Copy link
Contributor Author

Given the infrastructure making it hold three crates per tile would be a pretty distinct upgrade over simply attempting to smash crates via conveyor.

Another note on this: adding more (upgraded) crate storage racks is also an option, this can easily be done, even for different types - like barrels.

@github-actions github-actions bot removed the S: Merge Conflict This PR has conflicts that prevent merging label Mar 6, 2025
Copy link
Contributor

@whatston3 whatston3 left a comment

Choose a reason for hiding this comment

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

Without diving too deep into this:

  1. Let's say I have things bound for multiple places. How would I best use this in that case?
  2. Do we want to lean more into conveyor belts as the only real solution for moving goods? We have mechs, we have vehicles, why pick the most passive option?

Comment on lines +36 to +38
// Skip crate storage racks that aren't mounted on a grid.
if (compXform.GridUid == null)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed given 26-27, 40-41.

Comment on lines +34 to +35
if (!comp.Powered)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend using SharedPowerReceiverSystem.IsPowered rather than storing the powered state in the component.

Dirty(rackUid.Value, rackComp);

storedCrates.Add(new StoredCrate { CrateUid = crateUid, CrateStorageRack = rackUid.Value });
_transformSystem.SetCoordinates(crateUid, new EntityCoordinates(GetStorageMap(), Vector2.Zero));
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why use a dedicated map for this vs. nullspace?

}

// Dictionary where the key is the entity id of the crate storage and the value is the list of stored crates.
private readonly Dictionary<EntityUid, List<StoredCrate>> _storedCrates = new();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the components themselves to structure the interactions ("this machine knows about these crates, stored on these racks")? The entire fact that the entity ID is your key implies that you can store it per-entity without issue.

You can also:
A) clear it when the component shuts down (no more funny orphaned crates)
B) get the approximate value whenever some dingus sells their ship with 400 crates in off-map storage.

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.

4 participants