Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle unknown plane/helicopter types (mods) more gracefully/best effort #219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

magwo
Copy link
Contributor

@magwo magwo commented Apr 17, 2022

No description provided.

@magwo
Copy link
Contributor Author

magwo commented Dec 22, 2022

This became pretty relevant again when Black Shark 3 came out. It's a bit unfortunate/problematic that pydcs just breaks every time a new module comes out. Would be nice if such modules could just flow unchanged through the read/write scenario.

Copy link
Collaborator

@DanAlbert DanAlbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long. Overlooked this one but it came up again in #344.

I'm not really keen on treating things that are unknown so identically. The constructed type in this case will appear to be fully recognized and will have all the properties that a recognized aircraft would have, but all those properties would just be incorrect defaults.

If the behavior were controllable (for my use case, I want an exception raised, if there's something we don't recognize in the miz, something has gone wrong and we should stop processing) I wouldn't object, but my use case is very different from yours so @rp- is probably the one that should approve/deny (though from the other PR I can guess that there ought to be a test added that verifies that a miz that was deserialized in this manner still serializes correctly).


if imp_unit["type"] not in planes.plane_map:
print("WARN:", "Unknown plane type", file=sys.stderr)
plane_type = type(imp_unit["type"], (PlaneType,), {})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty wary of this approach:

>>> with open(os.devnull, "w") as dev_null:
...     pickle.dump(type("Bar", (Foo,), {})(), dev_null)
...
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
_pickle.PicklingError: Can't pickle <class '__main__.Bar'>: attribute lookup Bar on __main__ failed

I don't think it's important that a distinct type is created for each aircraft type, so PlaneType() is probably plenty good.

This also does call type() for every instance of the plane that isn't recognized. Experimentally that's not a problem in the repl (id(type("Foo")) == id(type("Foo")), but I can't find anything in the docs of type that specify the behavior one way or the other so I'd prefer to not rely on it without a good reason.

@rp-
Copy link
Collaborator

rp- commented Dec 12, 2023

I have currently not the time to fully rethink how unknown planes could be handled correctly.
But what would be important for me is that, we can load them and save them as they are and not edited in some way while saving. Until of course the user somehow edit values of the plane group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants