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

Materials used in engineering are not counted when syncing to Inara #1395

Closed
kiksu89 opened this issue Jan 8, 2022 · 15 comments
Closed

Materials used in engineering are not counted when syncing to Inara #1395

kiksu89 opened this issue Jan 8, 2022 · 15 comments
Labels
Milestone

Comments

@kiksu89
Copy link

kiksu89 commented Jan 8, 2022

Please complete the following information:

Describe the bug
When EDMC syncs material inventory to Inara, there seem to be some problems where the total amount is not counted correctly if materials have been used for engineering. The used material counts appear to be available in the journal events and EDMC logs seem to suggest that those events are processed, but the material amounts are not updated. This behavior seems to happen for both regular engineering and experimental effects. Only scooping or trading (aka gaining more) materials seem to update to Inara, and all this combined can throw the material amounts totally out-of-sync.

To Reproduce
Steps to reproduce the behavior:

  1. Go to a station and choose a pinned blueprint in Engineer's Workshop that you have materials for
    • I don't think it matters if you're at any station or at the engineers base
  2. Check what materials the blueprint requires and check the amounts in Inara > Commander > Inventory and Crafting > Inventory
    • Optionally, check that the amounts in Inara match the in-game amounts
  3. Use the blueprint at least once
  4. Trigger EDMC update
  5. Check if the material amount is updated in Inara
    • Give it a minute or two to update just in case

Expected behavior
Materials used in engineering should be retracted from the total amounts when sending them to Inara.

@kiksu89 kiksu89 added bug unconfirmed An unconfirmed bug labels Jan 8, 2022
@Athanasius Athanasius added this to the 5.3.0 milestone Jan 20, 2022
@Athanasius
Copy link
Contributor

  1. Do we already react to the pertinent events in monitor.py ? If not, we should. If we do, then is it correct for current Journal output?
  2. Do we already react to those same events in plugins/inara.py ? If not, we should be adding a message to the queue. Take care to remove any similar ones still in the queue, assuming the Inara message is taking the updated materials totals, rather than use deltas. If we already are, then find the apparent bug.

@Athanasius
Copy link
Contributor

This might be related to the following in monitor.py:

            elif event_type == 'EngineerCraft' or (
                event_type == 'engineerlegacyconvert' and not entry.get('IsPreview')
            ):

i.e. the first line needs the string folding to lower case.

@kiksu89
Copy link
Author

kiksu89 commented Feb 3, 2022

So, am I correct to believe that this has been confirmed on your end as well? Labels still say that this is unconfirmed.

And, even though I'm not an expert on Python, it does seem that event_type is converted to lower case just like you said. You must be on the right track and hopefully this is an easy fix.

@Athanasius
Copy link
Contributor

So, am I correct to believe that this has been confirmed on your end as well? Labels still say that this is unconfirmed.

And, even though I'm not an expert on Python, it does seem that event_type is converted to lower case just like you said. You must be on the right track and hopefully this is an easy fix.

Possibly, it'll certainly be the first thing checked when I'm not looking at other open issues (I came upon that code whilst checking something else).

@kiksu89
Copy link
Author

kiksu89 commented Feb 3, 2022

Possibly, it'll certainly be the first thing checked when I'm not looking at other open issues (I came upon that code whilst checking something else).

Understood.

@Athanasius
Copy link
Contributor

Athanasius commented Feb 4, 2022

4. Trigger EDMC update

Things to note about this step.

  1. Pressing the 'Update' button in the EDMC UI does not do a general "send current data/state through all the plugins, including ensuring Inara is updated about anything we're ever going to tell it about". All it actually does is trigger a pull of Frontier CAPI data (which will include station commodities, shipyard and outfitting if docked).
    In the case of Inara it will literally only send a setCommanderCredits message. Nothing else.
  2. Even if
    1. You perform actions that cause the game to log new Journal events
    2. AND one of those event types is deemed interesting with respect to EDMC's internal state tracking.
    3. AND those event(s) are deemed interesting in EDMC's Inara plugin.
    4. AND Inara has a matching API message for that data.
    5. AND EDMC's Inara plugin makes no attempt to limit how often it sends that particular Inara API message (i.e. we would never update for every change in Cmdr wallet balance, as the docs state not to do that).
      Then you might still have to wait up to 35 seconds (as currently configured in the EDMC Inara Plugin code) before a message is sent to the Inara API including that particular data.

Yes, we are aware it can make it difficult to check if it's just EDMC not having sent the data yet, or if some bug is preventing it being sent at all. We have that "at most every 35s" because if we send more than 2 messages a minute the Inara API will ban the user (identified by your API key) for one hour.

EDMarketConnector.exe --trace-on plugin.inara.events can be used to log the full detail of every message sent to the Inara API, but be aware this means that EDMarketConnector-debug.log will then contain your Inara API key, so be careful about sharing any log file that might contain such.

So, with those two points made....

Checking the code you can NOT expect EDMC to update Inara after every engineering action. You CAN expect Inara to be updated with EDMC's idea of your current materials if you relog. Given that the Inara API has delCommanderInventoryMaterialsItem, I'll look into changing the code so it does queue up a message for any engineering material use. (See below )

@Athanasius
Copy link
Contributor

Ref: #1433 , but that will be a part of 5.4.0 work.

@Athanasius
Copy link
Contributor

Checking the code you can NOT expect EDMC to update Inara after every engineering action. You CAN expect Inara to be updated with EDMC's idea of your current materials if you relog. Given that the Inara API has delCommanderInventoryMaterialsItem, I'll look into changing the code so it does queue up a message for any engineering material use.

I checked some more, and it turns out this is the case:

  1. EDMC Inara plugin records the currently tracked state of Materials for every journal event passed to it whilst sending to Inara is active.
  2. Then, again for every journal event it compares the last idea of that state to what was newly passed in, and if they differ at all it will send that new state to Inara, and update the record of 'last' state.

So, yes, now I've fixed the bug in the monitor.py event handling you should expect Inara to get updated for any engineering once at least 35 seconds have elapsed (and possibly sooner).

@Athanasius
Copy link
Contributor

Closing this for now, as it is believe fixed in the develop branch. Stay tuned for a new 5.3.0 beta release so you can test it.

@Athanasius
Copy link
Contributor

@Athanasius Athanasius reopened this Feb 5, 2022
@kiksu89
Copy link
Author

kiksu89 commented Feb 5, 2022

Please test https://github.com/EDCD/EDMarketConnector/releases/tag/Release%2F5.3.0-beta7

Downloaded, will test soon.

@kiksu89
Copy link
Author

kiksu89 commented Feb 5, 2022

So far so good, remote engineering is now reducing the correct amount of materials in Inara. Haven't gone to a specific engineer yet, but I assume it doesn't really differ as far as the journal goes.

Here's a snippet from EDMarketConnector.log when running with --trace-on plugin.inara.events after applying an engineer blueprint:

2022-02-05 12:05:41.305 UTC - TRACE - 7300:1508:1508 plugins.inara.new_worker:1555: Events:
{"header": {"appName": "E:D Market Connector", "appVersion": "5.3.0-beta7+8f9bfe90", "APIkey": "****", "commanderName": "kiksunator", "commanderFrontierID": "****"}, "events": [{"eventName": "setCommanderInventoryMaterials", "eventTimestamp": "2022-02-05T12:05:34Z", "eventData": [****materials here****]}, {"eventName": "resetCommanderInventory", "eventTimestamp": "2022-02-05T12:05:34Z", "eventData": [{"itemType": "Items"}, {"itemType": "Components"}, {"itemType": "Data"}, {"itemType": "Consumables"}]}, {"eventName": "setCommanderInventory", "eventTimestamp": "2022-02-05T12:05:34Z", "eventData": [****inventory here****]}]}

(sorry I couldn't figure out how to wrap this properly)

@Athanasius
Copy link
Contributor

Athanasius commented Feb 5, 2022

Re: line-wrap, that's just how "pre-formatted"/"code" comes out on GH. I did edit in the 'python' hint so it's now syntax highlighted though :)

@Athanasius Athanasius removed the unconfirmed An unconfirmed bug label Feb 5, 2022
@kiksu89
Copy link
Author

kiksu89 commented Feb 6, 2022

Just to keep the discussion alive, I haven't noticed any unexpected behaviour regarding the change that fixed this. The material amounts in Inara are now updated correctly when using engineering blueprints and adding experimental effects, just like they should be.

One thing does puzzle me though: how long has this issue existed and how has nobody else noticed this before? 🤔 I know I noticed the material amounts not matching a while ago already, but assumed it was some weird problem with Inara and didn't notice the cause until maybe a month ago. Am I just a rare nutcase that uses Inara to plan engineering and/or resource gathering?

@Athanasius
Copy link
Contributor

The code has been that way since mid-2021, so there's a clear 6 months in which no-one noticed and/or bothered to report it.

Thanks for checking this beta. I'll close this issue for good now you've tested it works.

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

No branches or pull requests

2 participants