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

[Dehardcode] Bunkerable dehardcode #1512

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

TaranDahl
Copy link
Contributor

@TaranDahl TaranDahl commented Jan 27, 2025

skip the hardcoded checks to static properties

Copy link

github-actions bot commented Jan 27, 2025

Nightly build for this pull request:

This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build.

Copy link
Member

@Metadorius Metadorius left a comment

Choose a reason for hiding this comment

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

I think a better way to implement that would be making Bunkerable=yes work on the combinations that are proven to be not causing major issues.

@TaranDahl
Copy link
Contributor Author

I think a better way to implement that would be making Bunkerable=yes work on the combinations that are proven to be not causing major issues.

What does “combination” mean 🤔 I don't quite understand you

@Metadorius
Copy link
Member

What does “combination” mean 🤔 I don't quite understand you

In your doc you said:

Skipping checks with this feature doesn't mean that vehicles and tank bunkers will interact correctly.

What I mean is that it's better to try different things on vehicles, like hover locomotor or anything that prevented Bunkerable=yes from working before, and if it's working correctly - then allow it, if it is bugged - then don't allow bunkering regardless.

@TaranDahl
Copy link
Contributor Author

What I mean is that it's better to try different things on vehicles, like hover locomotor or anything that prevented Bunkerable=yes from working before, and if it's working correctly - then allow it, if it is bugged - then don't allow bunkering regardless.

The original three checks in this function were no turret/no weapon/speed type is Hover. It is certain that they are all unreasonable.
I can test all the locos available to the vehicle and rule out the ones that don't work at all, but I don't know if there are any other flags besides loco that would invalidate the entry.
There are also some flags that can cause weird results. For example, a vehicle without a turret will rotate its body to target the enemy after entering; a vehicle with tunnel loco will enter the bunkered state in the incorrect position. I think these results should not be simply classified as bugs, maybe someone wants to use them. That's why I said in the documentation that it doesn't "interact correctly".

@TaranDahl TaranDahl requested a review from Metadorius January 28, 2025 01:35
@TaranDahl
Copy link
Contributor Author

I tried all loco. It looks like fly mech and hover can't get into the tank bunker.

Copy link
Member

@MortonPL MortonPL left a comment

Choose a reason for hiding this comment

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

I think that this change could be permanently enabled. Why does it need to be explicitly enabled? It won't affect vehicles that don't have Bunkerable=yes set anyway and it doesn't involve any costly logic.

docs/New-or-Enhanced-Logics.md Outdated Show resolved Hide resolved
@Aephiex
Copy link
Contributor

Aephiex commented Jan 29, 2025

I think that this change could be permanently enabled. Why does it need to be explicitly enabled? It won't affect vehicles that don't have Bunkerable=yes set anyway and it doesn't involve any costly logic.

If you mean Phobos will permanently change how bunkerability is checked, leaving no room for customization, despite that sounds like a blasphemy there isn't really any harm in it.

@TaranDahl
Copy link
Contributor Author

I think that this change could be permanently enabled. Why does it need to be explicitly enabled? It won't affect vehicles that don't have Bunkerable=yes set anyway and it doesn't involve any costly logic.

According to ModEnc, the default value of Bunkerable is yes. If enabled permanently, it will change the default🤔

@Aephiex
Copy link
Contributor

Aephiex commented Jan 30, 2025

According to ModEnc, the default value of Bunkerable is yes. If enabled permanently, it will change the default🤔

Ah, shikataganai. In that case there has to be a toggle for BunkerableAnyway.

@Aephiex
Copy link
Contributor

Aephiex commented Jan 30, 2025

I have a suggestion, to list in the docs all hardcoded checks for bunkerability and to tell which can be bypassed by BunkerableAnyway=yes and which can't be bypassed by it.

docs/New-or-Enhanced-Logics.md Outdated Show resolved Hide resolved
@TaranDahl TaranDahl requested a review from Metadorius January 31, 2025 03:00
@TaranDahl TaranDahl requested a review from MortonPL February 4, 2025 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants