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

Improve crumble block removal patches #465

Closed
wants to merge 2 commits into from

Conversation

dyceron
Copy link
Collaborator

@dyceron dyceron commented Sep 1, 2024

Completely removes the blocks if the configurations are set, instead of having them never respawn.

Surface
image

Area 1
image

Fixes #427

@dyceron dyceron requested a review from ThanatosGit September 1, 2024 20:44
@ThanatosGit
Copy link
Collaborator

But why?
That looks like a totally random and unnecessary thing to change from the behaviour it had for months now.
How is that improving anything?

  • Technically, we also named stuff like that remove_... in the schema, which is horrible to migrate
  • The description in the schema is wrong now
  • Requires to also change the description in randovania later

@dyceron
Copy link
Collaborator Author

dyceron commented Sep 2, 2024

My thought was it's cleaner in game as it actually removes the blocks lolenthe patches say, but I guess it's also a bit overkill for what it's doing. I can close this.

@Miepee
Copy link
Contributor

Miepee commented Sep 2, 2024

You could say that maybe it is more intuitive for people who play the rando for the first time and they didnt read the default settings.
But default preset starts with scan pulse anyway, and if they don't use it, their fault.

Copy link
Collaborator

@ThanatosGit ThanatosGit left a comment

Choose a reason for hiding this comment

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

As mentioned: The description in the schema.json needs an update, too.

I leave it to you if you want to change it or not. It's not like I'm opposed but I also don't really see a reson to change it.

bmssd = editor.get_file(f"maps/levels/c10_samus/{scenario_name}/{scenario_name}.bmssd", Bmssd)
for objects in object_group:
# Disable the blocks
bmsbk.raw["collision_cameras"][objects["cc_idx"]]["entries"].pop(objects["entry_idx"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know all these files by heart but pop looks like it is removing something instead of disable them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah removing them from the "master list" makes them not exist.

_update_blocks(editor, object_group, scenario_name)


def _update_blocks(editor: PatcherEditor, object_group: typing.Any, scenario_name: str) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

_remove_blocks maybe?
I always think that patch, update etc. is changing something instead of completly removing something.
Well, but I guess that remove is also kind of an update.

@dyceron
Copy link
Collaborator Author

dyceron commented Sep 3, 2024

I'm honestly unsure at this point if this is even worth doing. I'll leave this PR open for the time being.

@dyceron
Copy link
Collaborator Author

dyceron commented Sep 15, 2024

Decided this isn't worth doing at the moment. Could always revisit in the future depending on if we want to make an API to make edits for any block we want.

@dyceron dyceron closed this Sep 15, 2024
@dyceron dyceron deleted the crumble-block-patches-improvement branch October 20, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve block patches, by removing blocks outrigth
3 participants