Open rpdelaney opened 1 year ago
rpdelaney
commented
1 year ago In general, it would be nice to have less validation about unknown recipes, since this would enable working with mod recipes and entities without integrating state into the module.
redruin1
commented
1 year ago Currently this is intended behavior actually. It raises an error because such a blueprint would fail to be imported into vanilla Factorio, and I assume your Draftsman installation has none of the corresponding mods. You can install these mods in Draftsman itself to get access to these recipes, and it should provide you with the same validations that you would expect from normal, vanilla entities. You can simply copy the mods into Draftsman's installation location in the factorio-mods folder and then run draftsman-update and (providing the mods don't do anything I haven't accounted for) the module's data should update accordingly, allowing you to load the modded blueprint.
Now, depending on the circumstance I imagine this might still be inconvenient; you might want to just import a blueprint just to analyze its contents without having to install all mods associated with it. You can still load their raw dictionary with utils.string_to_JSON(), but that gets rid of all the other potentially useful validations.
The simplest solution would to be add a wrapper around the importing functions such that you can catch each error and handle them accordingly, while allowing the function to proceed as normal. If you have any ideas on what the best syntax for this would be, feel free to leave a comment.
Alternatively, it might make sense to distinguish loading vs. importing; so you can load a blueprint with "incorrect" recipes and entities, and then only when you try to create a blueprint string to import into Factorio with the incorrect mod configuration does it raise said errors. However, this would be a significant (and possibly breaking) API change, and since currently all validations are performed on Entity/Blueprint construction it would take some development cost to change this.
rpdelaney
commented
1 year ago I understand that doing this validation was an original design goal for draftsman, as you explained in your Alt-F4 blog and so on. I think the implementation you've chosen has some important drawbacks, though. Perhaps there is a middle ground? I'll offer a description of some of the issues, and some proposed solutions.
draftsman can't predict uses cases that involve a user intentionally putting the blueprint into an invalid state temporarily before reverting it to a valid state. For example:
Since I'm playing around with draftsman anyway, I thought this would be a chance to use it. So I copy-pasted my mall into a blueprint with the idea that I would use draftsman to quickly extract a list of recipes. However, I have some mod recipes in there, so I was unable to create the blueprint object until I removed those assemblers from the blueprint in Factorio. Did I make a mistake setting up draftsman? Maybe. But I don't need or want validation right now, so why do I have to deal with that?
That's a good catch, because I ran into this problem when using Nico's Factorio Solar Art tool to create a blueprint for a 100GW solar array that ended up being too big and I had to segment it into tiles manually. Nico's tool gave me a blueprint I couldn't use, and so I understand why you might not want your users to have that experience.
But look at it from the other side: I could use draftsman to do that segmentation much more cleanly, but only if you don't throw an exception immediately upon encountering a blueprint that Factorio won't accept. If you refuse to even create the object, I can't use draftsman to fix broken blueprints!
There's also the issue of maintaining state in the module itself. Sometimes, inevitably, draftsman is going to be wrong about what blueprints are "valid" and which aren't, from the perspective of the user.
Realistically, I'm not ever going to trust draftsman so blindly that I don't bother to place a blueprint in-game and actually look at it before I publish it to others. Would you? No. That means the benefits of rigid validation are questionable, in light of the costs.
If I make my own utilities and modules that depend on draftsman, I would want to test them using some kind of CI like Github Actions or other. But I can't put Factorio in CI. It's too big, and I may be exposed to liability for copyright infringement under U.S. copyright law.
Suggestions:
.is_valid, that a user can check if they want draftsman's opinion on whether a blueprint is valid. Edit: You can also cache this with functools so that you don't have to re-run the check if the instance hasn't changed since last time.As a user I feel this would achieve the best of all worlds. What do you think?
rpdelaney
commented
1 year ago Another cost I forgot to mention is performance. When performing numerous operations on a blueprint, continuously re-checking validity is expensive. This is another reason you should check validity when the user asks you to: if I want validation, I'll tell you when I'm ready for it :)
redruin1
commented
1 year ago 1.
Draftsman should not (as far as I can tell) require an installation of Factorio; it ships with Wube's public repo factorio-data as a submodule which has all the "vanilla" prototypes included, which is up-to-date. Calling draftsman-update runs the load process and stores the resulting data in pickle files in the draftsman/data folder. All of the data that Draftsman needs to run is included on installation, and is provided by Wube themselves, so no licensing issues.
If the criticism is more related to the two-step installation process itself (pip install factorio-draftsman followed by draftsman-update), this is really only done due to a technical reason (see this discussion) where I cannot call the script during the installation. This functionality is also not expected to be added anytime soon.
Alternatively, including a set of vanilla pickle (data) files in factorio-data alongside a regular distribution is a valid suggestion, though generated pickle files for one version of Python might not run on different versions of Python. Writing draftsman-update on installation is inconvenient, but it ensures the data will be readable by that version of Python and will actually work out "of the box".
The two-step process also has the side-effect of informing the user of the usage and purpose of the tool in the first place, as you would need to know the command to figure out how to add mods to the environment or when updating the factorio-data stored in the module. Though, due to the number of questions I've had on it, it might need a better explanation somewhere on it's functionality.
Now, I'm not married to storing the data in these pickle files; the only reason I'm using them was because the previous method was even worse. If a better method exists, please tell me about it; if it can solve the above while preserving the module's functionality I'll implement it in a heartbeat. But as it stands right now this problem is particularly hairy and comes with a number of asterisks, and draftsman-update solved the most problems while incurring the fewest drawbacks.
2.
This is a good idea, and reminds me of other discussions I've had in the past. Particularly the custom integration tests for Factorio mods. Thinking about it, this also shouldn't be particularly hard to implement.
Technically, this should also be possible even now: all of draftsman.data is both readable and writable:
# For example, assume a vanilla draftsman install
from draftsman.data import signals
from draftsman.entity import ConstantCombinator
my_new_signal_name = "my-new-signal"
my_new_signal_type = "virtual"
# Add the signals to the registry so we can call `signals.to_dict` from it
signals.type_of[my_new_signal_name] = my_new_signal_type
cc = ConstantCombinator()
cc.set_signal(index=0, signal="my-new-signal", count=100) # Raises no errorsHowever, when adding elaborate things such as entities you would want some help from the API, as they have many interconnected components, as well as sort orders in their data lists that draftsman-update enforces. In that sense, this would
have to be another major addition, similar to entity merging that came before it.
3, 4, 5
When I designed Draftsman, I always assumed that the user using the scripts (to generate something in Factorio) would necessarily have the same mods as the person developing the scripts. This led to the seemingly nice design pattern that if the script ran into unknown data, it would cause an error in Factorio, so it made sense to also issue said error in script. This prevents fatal import errors to potentially get "lost in the noise" of lesser warnings, and since Draftsman was designed around generation of Blueprints from scratch, if the script creates a blueprint that it cannot import into Factorio it has already kind of defeated it's point.
As the module has become more generic however, Draftsman uses are becoming more broad, and outputting of a valid blueprint string is not necessarily a requirement of a Draftsman script. What if the user just wants to analyze the blueprint itself without installing the mods it uses? How to fix a broken blueprint string when Draftsman won't let you import it? This makes interfacing with Draftsman for these purposes basically impossible.
So, going forward I should refine API like so:
Validation should remain a core Draftsman staple (and should probably remain on by default) but there's no reason we should take control away from the user and tell them that we know how their code works better than them. This would be a shift from the previous incarnation of errors being "issues that Draftsman thinks Factorio cannot resolve" to "issues that Draftsman itself cannot operate under". This would also add a significant performance benefit, as the user could assert that certain operations pose no issue and checking for validity during those instances would be redundant.
I think I'll set this as a milestone for 2.0: there are a number of smaller things that can be implemented in the downtime, and a useful API should be developed beforehand to ensure that user-experience is as good as it can possibly be.
bp string:
When reading this with
get_blueprintable_from_stringI am givendraftsman.error.InvalidRecipeError: 'flare-stack' not in this entity's valid recipes. This prevents me from reading blueprints with modded recipes, since the function does not return. I would prefer aWarningso that I can choose to continue.