OoliteProject / oolite

The main Oolite repository.
https://www.oolite.space
543 stars 70 forks source link

Witchjump js enhancements #450

Closed phkb closed 9 months ago

phkb commented 9 months ago

This PR is a QOL enhancement for witch space jumps. It adds the jump cause to all jump events, so each event can know what type of jump was performed. It also adds a new property to the player ship, "previousSystem", which holds the originating system ID from where the player has come. It will be in the range of -1 to 255.

On a programming note, I'm not happy with these lines: ShipScriptEvent(context, self, "shipWillEnterWitchspace", STRING_TO_JSVAL(JS_InternString(context, [[self jumpCause] UTF8String])), INT_TO_JSVAL([w_hole destination]));

Something feels off with the number of conversions I'm doing to pass the jump cause to the STRING_TO_JSVAL function. If there's a better way, please let me know! Ideally I'd like to return to the "NoCx" version of "ShipScriptEvent" (and therefore removing the requirement to grab the context), so if there's a way to do that as well, I'm all ears!

AnotherCommander commented 9 months ago

I think you are doing it right. This seems to be the way to convert a NSString to a JSVAL in other places in the codebase. The only observation I could make is that the last parameter of JS_InternString is a const char * and the conversion used elsewhere is of the form [nsstring cString] rather than [nsstring UTF8String]. In any case, I don;'t think it matters that much if you use the UTF8String way, maybe just consider if there is a specific reason for using it instead of cString. Do you expect the converted strings to be UTF8?

phkb commented 9 months ago

Based on the info here (https://developer.apple.com/documentation/foundation/nsstring/1497307-cstring?language=objc) it would seem that cString is deprecated, in favour of UTF8String. It certainly works as UTF8String, so I'll leave it that way for now. But very glad I was doing things (mostly) the right way!