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

WIP: SoundToUnit, LineStyles #344

Closed
wants to merge 0 commits into from
Closed

Conversation

gregretkowski
Copy link
Contributor

@gregretkowski gregretkowski commented Dec 3, 2023

I am contributing back several modifications I made as part of using this library with our squadron's training mission file, that can be found at https://github.com/gregretkowski/OA-Syria_Training_Map - note the lib is not fully loading my mission yet but this PR provides fixes for two issues found while loading the miz.

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.

(please squash the changes into the appropriate commit when you upload them, otherwise I can't merge this cleanly)

dcs/coalition.py Outdated
plane_group.add_unit(plane)
except KeyError as e:
msg = f"Error loading Plane, skipped loading it from miz - is it a mod?: {e}"
print("ERROR", msg, file=sys.stderr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

logging.exception?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have another concern here, if you save this .miz file, the plane group will be gone.
So I'm not sure this should go on master, and rather fixed with some new mechanic that allows to at least preserve
modded/unknown planes/helis/vehicles....

Or have some load flag, to allow best effort loading, that allows to remove unloadable data or something like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The error behavior should maybe be configurable with a flag to the mission load? I went back and forth on this a few times before deleting my comment as YAGNI. I haven't decided how I'd actually want this to work in Liberation. In our case we only deserialize for campaign definitions, and if a campaign designer has put some stuff in there that we can't load, that's maybe a sign that they made a mistake and we'd be ignoring it rather than signaling that there's a bug? (yes, it'll log an error, but people don't typically read logs IME)

(in any case, @gregretkowski, if you split the rest of your changes out into a separate PR, the rest is trivially correct so I'd merge that now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my last change where I realized I wont be able to use this lib for what I wanted to do with my miz, but I got this far so I opened the PR.. I looked at how planes are defined in the lib and it looks like they are exported as very detailed data structures, afaict putting in support for them would be a large effort - and with this change above while it helps to load the mission its not going to help someone save the mission afterwards as the modded planes will be missing.

I'll remove the 'modded plane' part of this PR and just have it contain the SoundToUnit, LineStyles changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case you may want to subscribe on #247. Not sure if it'd solve your problem or not but part of the goal is to reduce just how my pydcs needs to know about things. I haven't made any progress on that in a while, so it won't solve your problem any time soon, but it doesn't hurt to subscribe :)

dcs/coalition.py Outdated Show resolved Hide resolved
@gregretkowski gregretkowski changed the title SoundToUnit, LineStyles, skipping modded plane loads WIP: SoundToUnit, LineStyles, skipping modded plane loads Dec 8, 2023
@DanAlbert
Copy link
Collaborator

(please squash the changes into the appropriate commit when you upload them, otherwise I can't merge this cleanly)

Hmm. I was going to force-push to fix the git history in your branch so I could merge it, but the PR comes from your master branch, and I suspect you don't want me rewriting that history. lmk how you want to handle that, but I would prefer to merge this as distinct commits since you nicely organized them as such, but to do that the unknown plane commit needs to be removed from the history. In other words, the PR should have these commits:

~\src\pydcs [greg-pr]> git log --oneline
3ceacca (HEAD -> greg-pr) Update action.py fix set_ai_task obj name
9a7d908 Update action.py fix set_ai_task
065c820 actions_export.py SoundToUnit
76e74d7 add action sound to unit
8cbae21 Add strongpoint and others to LineStyle

That's a trivial fix if you don't mind me force pushing to your master branch. I think the alternative is that we open a new PR from a different branch? (which I can do if you'd prefer)

(btw, for this reason I find it's usually easier to send multiple PRs: it's impossible to merge only part of a PR, and the more things there are in a single PR the more likely that something is going to need to change)

@DanAlbert
Copy link
Collaborator

(checks are also still failing)

@DanAlbert
Copy link
Collaborator

btw for the other problem I see there's also an old and overlooked PR #219 that aims to do similarly, but with a different approach (that I haven't looked at so idk if it has the same problems or not).

@gregretkowski gregretkowski changed the title WIP: SoundToUnit, LineStyles, skipping modded plane loads WIP: SoundToUnit, LineStyles Dec 18, 2023
@gregretkowski
Copy link
Contributor Author

Just pushed what I hope will fix the linter error. Feel free to do anything you need to (force push or whatever) to my fork of the repo - I can just re-sync my fork when I do additional work on the code. I hoped to fix up the pr more over the weekend but been to swamped to do so.

@DanAlbert
Copy link
Collaborator

I pushed the wrong ref which force closed this and I can't reopen it, so I just uploaded a new PR: #347

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