Phobos-developers / Phobos

Ares-compatible C&C Red Alert 2: Yuri's Revenge engine extension
GNU Lesser General Public License v3.0
299 stars 95 forks source link

More DebrisTypes than MaximumDebris causes Desync #1165

Open Danielovich7 opened 1 year ago

Danielovich7 commented 1 year ago

Description

If there is more debris listed in DebrisTypes= than there are number of debris listed in MaximumDebirs=, it causes desync. That was the desync issue in Red Alert 20XX pre-1.0.6b. Removing the too many debris in DebrisTypes= fixed the issue.

Conditions to reproduce

On a TechnoType, add more debris in DebrisTypes= than available MaximumDebris=. Make sure they are Voxel debris.

INI code

...
DebrisTypes=DBRIS1, DBRIS2
DebrisMaximums=1
...

Steps to reproduce

  1. See above
  2. ...

Expected behaviour

A warning in debug.log, maybe with somehow sanitized value which would not cause a desync.

Actual behaviour

It causes desync.

Additional context

No response

Checklist

Otamaa commented 1 year ago

Game using index based on DebrisTypes items count , ofcourse it will desync because it will do out of bound array access for the DebrisMaximums especially when DebrisMaximums items count is less than DebrisTypes items count , so this is not an bug but user error , as you can see on this address 0x0007023C8 for the details.

Metadorius commented 1 year ago

Game using index based on DebrisTypes items count , ofcourse it will desync because it will do out of bound array access for the DebrisMaximums especially when DebrisMaximums items count is less than DebrisTypes items count , so this is not an bug but user error , as you can see on this address 0x0007023C8 for the details.

Well probably the report could've been a bit clearer, but regardless the point is that it should not just silently fail somewhere. The point is that there should be either a clear warning that this causes a desync or maybe even a sanitized value which would not. Or the game should outright refuse to work until that is fixed.

Otamaa commented 1 year ago

Game using index based on DebrisTypes items count , ofcourse it will desync because it will do out of bound array access for the DebrisMaximums especially when DebrisMaximums items count is less than DebrisTypes items count , so this is not an bug but user error , as you can see on this address 0x0007023C8 for the details.

Well probably the report could've been a bit clearer, but regardless the point is that it should not just silently fail somewhere. The point is that there should be either a clear warning that this causes a desync or maybe even a sanitized value which would not. Or the game should outright refuse to work until that is fixed.

well , i agree that it problably just fail or maybe should put warning somewhere . should we do TechnoType evaluation like ares does ? so we can put more warnings in case user doing something that not suppose to be ,..

Metadorius commented 1 year ago

Since Ares is stale - it will probably be a good idea to do so. The behavior I see best is emitting a developer warning and automatically fixing the value to closest valid one.

Otamaa commented 1 year ago

Since Ares is stale - it will probably be a good idea to do so. The behavior I see best is emitting a developer warning and automatically fixing the value to closest valid one.

https://github.com/Ares-Developers/Ares/blob/4f1d929920aca31924c6cd4d3dfa849daa65252a/src/Misc/Invalidators.cpp#L65C35-L65C35