Revolutionary-Games / Thrive

The main repository for the development of the evolution game Thrive.
https://revolutionarygamesstudio.com/
Other
2.84k stars 506 forks source link

Add button to delete fossils in the museum #3930

Closed hhyyrylainen closed 1 year ago

hhyyrylainen commented 1 year ago

as there doesn't seem to be a way to do that other than going to the folder to delete things manually, seems like this is an oversight.

Brokemia commented 1 year ago

Going to try doing this as an easy practice issue. Should there be an "Are you sure you want to delete this?" popup?

hhyyrylainen commented 1 year ago

I guess there should be such confirmation as it is an irreversible operation that permanently loses data.

Oliveriver commented 1 year ago

This could be either a button in the right hand species display, or for something more compact, a cross in the top right of the species card.

Brokemia commented 1 year ago

I've done a quick mockup of both options. I think I prefer the larger delete button on the right. Although my issues with the other option might just be because there's no border around the card, so having a border on the trash icon looks inconsistent image

Brokemia commented 1 year ago

image Hmm, not sure this helped

adQuid commented 1 year ago

@Nunez2196 your thoughts?

Nunez2196 commented 1 year ago

I much prefer the button without a border personally.

Brokemia commented 1 year ago

To delete a fossil, I think I need to track the file name somewhere related to the MuseumCard. It's returned as part of the FossilisedSpecies returned from FossilisedSpecies.LoadSpeciesFromFile(speciesName), but the name is ignored and just the species and preview image are given to the card. Should I just have the card also have a Name property for this purpose?

adQuid commented 1 year ago

That sounds like a good idea for a first pass. Someone more experienced in that area of code might suggest something better when reviewing the PR.

athariqk commented 1 year ago

@Brokemia I like the trash button in the latest picture, I think it should only be visible when you mouse hover one of the cards though. On the fossil deletion, Species already contains FormattedName property which you can just use right away, that's how the MuseumCard's name label got it.

Brokemia commented 1 year ago

FormattedName isn't guaranteed to be the same as the file name. I don't think the game will ever make them anything different at the moment. Although as a side note, I would argue it should be possible to save multiple fossils with the same name (but different file names, maybe just by adding a number at the end) in case a player wants to record how their species changed, or if they just don't want to be bothered to change the name from the default.

But more relevantly to the current state of the game, if a player manually renames the file, it'll load just fine and thus FormattedName won't match the file name

athariqk commented 1 year ago

Fair point, I forgot that players can freely change their fossils name in the fossilization dialog. Then yes, I think it would make sense to store the file name in the card either as a usual C# property or Godot's Object metadata.

hhyyrylainen commented 1 year ago

I don't think Godot metadata is used anywhere else so it shouldn't be used for just this one feature.

athariqk commented 1 year ago

It does used here apparently: https://github.com/Revolutionary-Games/Thrive/blob/c9dc42df6904a1d19cb776ee909e30f362dc7d28/src/thriveopedia/Thriveopedia.cs#L273

Besides it's just extra convenient so why not.

hhyyrylainen commented 1 year ago

I see, maybe it'd be better to refactor that also away? https://github.com/Revolutionary-Games/Thrive/issues/3944

Brokemia commented 1 year ago

Ok, so I have this mostly working. Just need to add the localization stuff and the delete button image (which still has the border right now). I don't really understand how to add the localization text. The setup instructions say to use Pybabel, but they don't really elaborate on that and the readme doesn't do much better.

GameDungeon commented 1 year ago

This should help. https://github.com/Revolutionary-Games/Thrive/blob/master/doc/working_with_translations.md

Brokemia commented 1 year ago

Thanks, that's exactly what I was looking for. PR inbound