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

Only remove -vis from name if it is part of the name. #3257

Merged
merged 5 commits into from Feb 24, 2024

Conversation

LuminarLight
Copy link
Contributor

During level extraction, the last 4 characters of the level name are always removed, because it is assumed that '-vis' is there. But it isn't always there. This is especially true in post-TPL games.

This causes multiple problems:

  • There can be levels in the extraction that will miss their last 4 characters from their name, which is sad, and may make it harder to identify them.
  • If there are '-vis'-less levels whose names are identical apart from the last 4 characters, the extractor will only get the last one (it probably extracts all but overwrites everything but the last one). For example 'ctyasha' and 'ctykora'.

This issue affects the glb extraction and the entities json extraction.

I personally think that just keeping the -vis in the name would be the best solution, but I guess there was a reason why it was decided that it should be removed from the name. So to adapt to this, my implementation will still remove '-vis' from the name, but only if it is actually in the name - otherwise it won't remove anything.

I hope my changes didn't break anything. Extraction seemed to run fine after my changes, and I was able to see both ctyasha and ctykora json files. And didn't see any '-vis', so it is still properly removed.

@OpenGOALBot
Copy link
Collaborator

Can one of the admins verify this patch?

@ManDude
Copy link
Member

ManDude commented Dec 12, 2023

Alternatively, just trim the last 4 characters of the visname.

Honestly I don't think the name should be trimmed at all, now that I look at it.

@LuminarLight
Copy link
Contributor Author

Alternatively, just trim the last 4 characters of the visname.

Honestly I don't think the name should be trimmed at all, now that I look at it.

Please let me know what the team thinks. As I said I also think that this '-vis' removal is just an extra headache, but maybe there was a reason why it was done like that.

@xTVaser
Copy link
Member

xTVaser commented Feb 24, 2024

@LuminarLight Remove the logic that trims the name at all and this can be merged
image

@xTVaser xTVaser marked this pull request as draft February 24, 2024 00:04
@LuminarLight
Copy link
Contributor Author

I removed the level name trimming, but now extraction fails. There may be parts of code that expect the de-vised name. I did some investigation but couldn't figure out where it goes wrong.

@xTVaser
Copy link
Member

xTVaser commented Feb 24, 2024

ok to test

@xTVaser xTVaser marked this pull request as ready for review February 24, 2024 18:57
@xTVaser xTVaser merged commit 0ae0938 into open-goal:master Feb 24, 2024
9 checks passed
@LuminarLight LuminarLight deleted the devisonlyiftherevis branch February 24, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants