More of the same - ToCitizenInstance #1400
Conversation
Added more convenience methods to CitizenInstanceExtensions.
DaEgi01
added
the
code cleanup
DaEgi01
requested review from
kvakvs,
originalfoo,
krzychu124 and
kianzarrin
| @@ -63,7 +63,8 @@ public class AdvancedParkingManager | |||
| case ExtPathMode.DrivingToTarget: { | |||
| // citizen instance requires a car but is walking: release instance to | |||
| // prevent it from floating | |||
| if ((Singleton<CitizenManager>.instance.m_instances.m_buffer[(ushort)citizenInstanceId].m_flags & CitizenInstance.Flags.Character) != 0) { | |||
| ref CitizenInstance citizenInstance = ref ((ushort)citizenInstanceId).ToCitizenInstance(); | |||
There was a problem hiding this comment.
Would it be worth adding an extension that accepts this uint citizenInstanceId and automatically casts to ushort internally?
internal static ref CitizenInstance ToCitizenInstance(this uint citizenInstance)
=> ref _citizenInstanceBuffer[(ushort)citizenInstance];There was a problem hiding this comment.
mmmh shit ... now that I thought about it and double checked, this casting actually suxx bad and could lead to errors.
valueoutside_ushort casted to ushort will always produce a ushort, even for values outside ushort. it will just overflow and by default overflow is not checked :/
So we can either do the checked {} thing, which will cost us some perf, or we can just hope for the best, in which case it does not matter if I do cast or don't at all it seems to me :/
There was a problem hiding this comment.
Ok so I added the uint extension method but without the cast.
As far as I can tell array indexes are always int. In .net framework for sure, in Mono I did not find the code XD
Which means that all values given to it will be coverted to int anyway.
Precasting it would cause errors in case we give it a value > ushort.MaxValue and we get an overflow.
So I'd rather rely on an IndexOutOfRangeException in case we mess up instead getting an overflow, not realizing it and manipulating the wrong data.
Please correct me if you think my logic is wrong :D
| for (int i = 1; i < CitizenManager.MAX_INSTANCE_COUNT; ++i) { | ||
| if ((citManager.m_instances.m_buffer[i].m_flags & | ||
| CitizenInstance.Flags.Character) == CitizenInstance.Flags.None) { | ||
| for (uint citizenInstanceId = 1; citizenInstanceId < CitizenManager.MAX_INSTANCE_COUNT; ++citizenInstanceId) { |
There was a problem hiding this comment.
Does this get resolved at compile time? If so, will it not cause problems at later date if a mod somehow extends number of citizen instances? Should we instead query m_length (or whatever) on the buffer?
There was a problem hiding this comment.
Yeah, I remember there was a PR that fixed that? Or so I thought. Maybe for other constants only.
There was a problem hiding this comment.
if field is not const and just readonly then it shouldn't get resolved
There was a problem hiding this comment.
Anyway, its not part of the "ToCitizenInstance". But we could and should to a separate refactoring here to remove those constants and make a runtime call instead.
# Conflicts: # TLM/TLM/Patch/_VehicleAI/_PassengerCarAI/GetLocalizedStatusPatch.cs
…und. Some minor cleanups here and there.
Converted more lines to ToCitizenInstance().
Added more convenience methods to CitizenInstanceExtensions.