Yes, that was my reaction also. Similar arrays are elsewhere presented as read-only propertiesAhruman wrote:This does seem rather inconsistent.
Seeing your changes, I now have an idea why you didn't do it immediately: There was no property used in the mission object until now.cim wrote:Since I can't remember why I did it that way, changed to a property in r5217 before anyone other than Eric starts using it...
Using new stuff in trunk is valuable as it reveals bugs or inconsistencies before it enters a release version.
I would propose one change though:
With the current implementation, all old oxp's keep working as before. And new ones the immediately start with the new code will have no problems either. Problem will be the existing oxps that want to switch to using the new markers. They have two options:
1) Just use the new markers and accept that the old ones are not removed until the player jumps to a new galaxy.
2) Add code to convert the old ones.
1 will be enough for most mission oxps, but if you want to convert the markers, the method with 'markedSystems' is a bit overkill to do it every time you want to remove a marker. Specially because you have to loop through all the marked systems till you get a hit.
To avoid that and only do it once, you can introduce a new mission variable to remember you made a change.
After playing with the code of two oxp's I came to the conclusion that it would be simpler if the 'mission.unmarkSystem' would be a boolean that returned false when it failed to unmark a system. An oxp should know if that marker must be there and can conclude on a failure that there must be a legacy marker on that spot.
So, I would suggest to change 'mission.unmarkSystem' to a boolean.