BenChung / KRPC.jl

A kRPC client for Julia
MIT License
3 stars 4 forks source link

Fix string getter and enumeration of parts #8

Closed Rhahi closed 2 years ago

Rhahi commented 2 years ago

This PR addresses two problems I found so far: getting string, and getting "Parts".

I am new to Julia, and I do not really understand what is going on behind the scenes, but here is what I did to fix the problem.

Problem 1: failure to get strings

The problem is caused from types.jl:108, where Julia is asking for String, but there is no method for getJuliaValue for String. There is one for AbstractString, though. So I added it to existing AbstractString.

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Union{Type{AbstractString}, Type{String}})
    return ProtoBuf.read_string(PipeBuffer(value))
end

This will enable the user to use commands like Title(part), Name(part).

Problem 2: failure to get parts

I am expecting that similar problems will pop up more as I try to access more of the KRPC entities. I was not able to get parts of the vessel by using Parts(vessel), where vessel is KRPC.Interface.SpaceCenter.RemoteTypes.Vessel.

The solution to this problem is twofold:

types.jl:131 has call for getJuliaValue, but omits argument for conn. Adding conn fixes this problem. I notice that line 98 may also have a similar problem, but I did not touch it since I didn't need to use this method (yet)

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Type{Array{T,1}}) where T
    res = readproto(PipeBuffer(value), krpc.schema.List())
    return T[getJuliaValue(conn,item,T) for item in res.items]
end

And then there should be a getJuliaValue method for the RemoteTypes.Part type itself! This is the part where I am writing something I don't understand.

in a brand new file, which I called aftertypes.jl, I created a new method that will return the correct part. because it references types added in generated.jl, I cannot add this method in types.jl, hence the new file. I took the lines from other method that seems to do something similar, and splashed a Part into it. This code might make no sense! If you see that something is off, please patch that in if you are merging this PR.

import KRPC.Interface.SpaceCenter.RemoteTypes as RemoteTypes

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Type{RemoteTypes.Part})
    res = ProtoBuf.read_varint(PipeBuffer(value), Int64)
    if res == 0
        return Nothing()
    else
        return RemoteTypes.Part(conn, res)
    end
end

I am expecting that similar patches may be needed to extend kRPC to julia even more. Let me know if these kind of PRs are welcome.

/Rhahi

BenChung commented 2 years ago

Yes, the other issues you've highlighted are bugs. If you want to fix the issue at line 98 too I can merge that in.

One note there on the getJuliaValue of a String problem.

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Union{Type{AbstractString}, Type{String}})

would be better written as

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Type{T}) where T <: AbstractString

type constructor parameters (e.g. the T in Type{T}) are invariant in Julia, so Type{AbstractString} only matches the literal type AbstractString. You can see this with:

julia> String isa Type{String}
true

julia> String isa Type{T} where T<: AbstractString
true

julia> String isa Type{AbstractString}
false

by using the where T<:AbstractString that makes Julia use it wherever the parameter T is a subtype of AbstractString. Sorry about that.

And then there should be a getJuliaValue method for the RemoteTypes.Part type itself! This is the part where I am writing something I don't understand.

That should be handled by types.jl:149 and onwards, in particular

function getJuliaValue(conn, value::Array{UInt8, 1}, rtype::Type{Union{T, Nothing}}) where {T<:kRPCTypes.Class}
    res = ProtoBuf.read_varint(PipeBuffer(value), Int64)
    if res == 0
        return Nothing()
    else
        return T(res)
    end
end

In this case, since Part is a subtype of kRPCTypes.Class (as defined in generated.jl with struct Part <: kRPCTypes.Class), then this version of getJuliaValue will be applied. It looks like I made the same bug again, wherein if res is nonzero then the constructor T should be invoked with conn as well as the index. I'll check this, give me a moment.

BenChung commented 2 years ago

Ah, okay, in my testing the existing code works for Part:

spaceCenter = KRPC.Interface.SpaceCenter.RemoteTypes.SpaceCenter(conn)
vessel =KRPC.Interface.SpaceCenter.Helpers.ActiveVessel(spaceCenter)
parts = KRPC.Interface.SpaceCenter.Helpers.Parts(vessel)
allparts = KRPC.Interface.SpaceCenter.Helpers.All(parts)

works for me (assuming that conn has been set up). This is because it doesn't go through parts.jl:149, but instead uses parts.jl:101 (which correctly passes the conn parameter). The version at line 149 is less specific (since it's a union of types instead of just a type) and thus Julia will only use it in case the type provided is actually that union. As a result, it isn't used for just getting Parts.

However, line 149 is still missing the conn parameter. I'll merge this and make the additional fixes you mentioned.

BenChung commented 2 years ago

Oh, and if you're still having the problem with getting Parts please post an example I can test the fix with.

Rhahi commented 2 years ago

No problem with Parts so far! There are some minor hiccups but none that restarting a server didn't fix.