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.

Sign up for GitHub

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

Jump to bottom

randomize iconSmoothing #28158

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

randomize iconSmoothing #28158

wants to merge 8 commits into from

Conversation

TheShuEd
Copy link
Contributor

@TheShuEd TheShuEd commented May 20, 2024 •
edited

added ability to randomize BaseState for IconSmoothing, this allows you to make walls with random states, like this:

image

https://discord.com/channels/310555209753690112/310555209753690112/1242119168606732359

@TheShuEd TheShuEd mentioned this pull request May 20, 2024
Closed
@TheShuEd
Copy link
Contributor Author

I have no idea why the test is falling. In my fork it pass all tests

@TheShuEd
Copy link
Contributor Author

Bruh ok

moonheart08
moonheart08 reviewed May 20, 2024
View reviewed changes
Content.Client/IconSmoothing/IconSmoothSystem.cs Outdated
Comment on lines 50 to 51
if (component.RandomStates.Count > 0)
component.StateBase = _random.Pick(component.RandomStates);
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 check if StateBase has already been set (so loading initialized maps preserves the existing state)

Ideally, this would be randomized at mapinit instead as well, but that seems to be a taller ask.

Copy link
Contributor Author

@TheShuEd TheShuEd May 21, 2024 •
edited

Choose a reason for hiding this comment

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

StateBase always set. its have default "base" value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also its client-side component and system, server dont save this data

Copy link
Contributor

Choose a reason for hiding this comment

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

The randomization should probably be shared, then (Separate component?). Otherwise clients will get inconsistent visuals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason why I don't want to add this to the shared is that storing random information from RandomSprite on the server loads all the objects on procedurally generated biomes, and clogs up memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's that big of an issue, especially when the alternative is inconsistent (per client) visuals.

@github-actions github-actions bot added the Changes: Sprites Should be reviewed or fixed by people who are knowledgeable with spriting or visual design. label May 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 21, 2024 •
edited

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

Resources/Textures/Structures/Walls/mining.rsi

State Old New Status
miningB0 Added
miningB1 Added
miningB2 Added
miningB3 Added
miningB4 Added
miningB5 Added
miningB6 Added
miningB7 Added

Edit: diff updated after 1dca508

Tayrtahn
Tayrtahn reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@Tayrtahn Tayrtahn left a comment

Choose a reason for hiding this comment

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

Not doing a full review right now, but this should fix the error.

Content.Server/IconSmoothing/RandomIconSmoothSystem.cs Outdated Show resolved Hide resolved
@TheShuEd
Copy link
Contributor Author

It's not working for some reason so far, I don't understand why yet.

@github-actions github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label May 24, 2024
@TheShuEd TheShuEd marked this pull request as draft May 24, 2024 18:05
Tayrtahn
Tayrtahn reviewed May 25, 2024
View reviewed changes
Copy link
Contributor

@Tayrtahn Tayrtahn left a comment

Choose a reason for hiding this comment

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

You're gonna be mad about this one.

There's another error now with LayerMapSet getting upset about trying to add the same thing twice, but this is progress!

Content.Client/IconSmoothing/ClientRandomIconSmoothSystem.cs Outdated Show resolved Hide resolved
@TheShuEd TheShuEd marked this pull request as ready for review May 25, 2024 15:37
@TheShuEd
Copy link
Contributor Author

image

it works

The mining walls now have a second version, slightly more dilapidated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tayrtahn Tayrtahn Tayrtahn left review comments

@moonheart08 moonheart08 Awaiting requested review from moonheart08

Assignees
No one assigned
Labels
Changes: Sprites Should be reviewed or fixed by people who are knowledgeable with spriting or visual design. Status: Needs Review This PR requires new reviews before it can be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@TheShuEd @Tayrtahn @moonheart08