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

paper is now burnable #10373

Closed
wants to merge 5 commits into from

Conversation

JustinTrotter
Copy link
Contributor

@JustinTrotter JustinTrotter commented Aug 6, 2022

About the PR

This started by wanting to Implement Charcoal #10042. To begin with it I wanted to have a easy way for the players to make ash. So down the rabbit hole I went and found myself playing with fire. So now you can burn papers. It works fairly well, you can burn whole piles of paper with any isHot tool. Fire can spread to nearby papers but they currently have to be really damn close to one other.

Things I would like to improve (now or later):

  • Expand this to other burnable entities like wooden chairs, tables, floors, etc. However, I found myself stuck on getting the fire visual sprite to render on the tables. I really could use some help on identifying what is wrong.
  • Spread the fire to nearby Flammable components, I could do this with some sort of collision or by checking the nearest tile.
  • Fix the fire extinguisher to actually put out the fires.
  • Balance the burn time with its base material
  • Spread the fire to mobs on collision, right now this would work except then they wouldn't be able to put themselves out

Screenshots

Screenshot_2022-08-06_15-03-41
Screenshot_2022-08-06_15-03-53
Screenshot_2022-08-06_15-03-59
Screenshot_2022-08-06_15-04-15

Changelog

🆑

  • add: papers can be set on fire

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted labels Aug 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2022

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

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Aug 6, 2022
Comment on lines 335 to 350
if(transform.GridUid != null)
{
var tile = _transformSystem.GetGridOrMapTilePosition(uid, transform);
var temperature = 700f;
var volume = 50f;
_atmosphereSystem.HotspotExpose(transform.GridUid.Value,
_transformSystem.GetGridOrMapTilePosition(uid, transform),
700f, 50f, true);
tile,
temperature, volume, true);

// Spread the fire to other flammables on the same tile
var fireEvent = new TileFireEvent(temperature, volume);
foreach (var entity in _lookup.GetEntitiesIntersecting(transform.GridUid.Value, tile))
{
RaiseLocalEvent(entity, ref fireEvent, false);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very suss. Instead of the flammable relaying itself to atmos it's relaying directly to other flammables in an expensive loop every tick.

@metalgearsloth
Copy link
Contributor

@Zumorica

@JustinTrotter
Copy link
Contributor Author

I've given up on burn chaining for this PR and have opted to just implement basic paper burning instead.

Maybe later I'll try my hand at physics and fixtures again to figure out how to chain burning papers.

Comment on lines +116 to +119
if (flammable.Ignitable)
{
flammable.FireStacks = Math.Max(2, flammable.FireStacks);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be getting handled in Ignite, not only interact using. IMO Ignitable should be renamed to be something like AlwaysCombustible, since I think that fits better what this is going for

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, an even better solution would be to just make FireStacks serializable, and add an option to prevent FireStacks from ever decreasing (then just give paper permanent firestacks), which 1) has this same behavior in a more intuitive way and 2) allows more granularity with how 'always capable of being on fire' something is

@mirrorcult mirrorcult added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Sep 6, 2022
@metalgearsloth metalgearsloth added the S: Derelict Status: Abandoned, but may contain something that can be salvaged. label Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Awaiting Changes Status: Changes are required before another review can happen S: Derelict Status: Abandoned, but may contain something that can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants