Sometimes, Dream Maker just doesn't get rid of extra delay information when a state has the number of frames edited.
This means we need to truncate our delay list to the number of frames specified by the frames key.
This always worked fine- however, we also simplify `delays = 1,1,...` to `Frames::Count(delays.len())`.
The bug in our code was that we checked if our `delays = 1,1,...` *before* truncating the array in the truncation case, so we would output `Frames::Delays([1,1])` for this metadata.
Caused a PartialEq failure in IDB here:
https://github.com/tigercat2000/IconDiffBot2-test/pull/20https://github.com/ParadiseSS13/Paradise/pull/18326
This is finally something I'm comfortable with having added upstream, the API is stabilized and functions well for IDB2's use-case, but should be able to be used by other projects as well.
New dependencies:
- Added the `either` crate to get around an annoying type issue with chained iterators.
- Added the `derivative` crate to make `State` realistically comparable across different icon files (important for diffing in IDB2)
- `offset` is ignored for PartialEq.
Notes on public API changes:
- Most of the new stuff is in the inline module `dmm_tools::dmi::render`.
- `IconFile`, `Rgba8`, and `Image` are now `Debug`
- `from_bytes` on `Image`, `IconFile`, and `State` no longer takes an explicit &[u8] array, instead it takes the trait bound `<B: AsRef<[u8]>>` for ease of "eat my ownership because I don't care"
- `IconFile` has a new function `pub fn get_icon_state<S: AsRef<str>>(&self, icon_state: S) -> io::Result<&State>` for looking up states by name.
- `Image` has a `clear()` method which fills the data array with defaults.
- `State` has a new member, `pub duplicate: Option<u32>`, which is used for correctly supporting duplicate icon states by enumerating how many times the state name has been seen before this particular `State` was read.
- `State` has a `get_icon_state` method which backs `IconFile`'s `get_icon_state`.
- `State` has a `is_animated(&self) -> bool` method which is used in IDB2 for precalculating the file extension
- `State` also has `get_state_name_index`, which is used by `parse_metadata` to give duplicate states a unique entry into the `state_names` `BTreeMap`.
- `parse_metadata` now keeps track of duplicate states and puts them in `state_names` according to the above.
Specifically, add `#[derive(Clone, PartialEq)]` to `dmi::{Metadata, State, Frames}`
As per (very weakly authoritative) <https://rust-lang.github.io/api-guidelines/interoperability.html> one should derive all the std's traits whenever feasible. Currently for my own project I only need `State::Clone`. The `PartialEq` is being added for consistency with it being present on other structs already, and the rest of the `std`'s traits feel a bit like an overkill presently (despite what the guidelines might say)
Currently parsing a malformed - but also well-formed, yet ancient - .dmi files results in a panic deep within the guts of SpacemanDMM. That's undesirable, since `.dmi`'s which SpacemanDMM does not understand (yet BYOND dreammaker handles fine) do exist in the wild (see https://github.com/ParadiseSS13/Paradise/pull/17800 for a couple of examples).
I don't think teaching SpacemanDMM of the legacy file formats is worth it, but making it return a Result instead of outright panicking definitely is (and it's much less work than the former).
This is technically a breaking change since it changes the signature of the public `dreammaker::dmi::Metadata::meta_from_str()`. (But it's probably rarely used, and definitely easy to fix at the call site?..)
Co-authored-by: Tad Hardesty <tad@platymuus.com>
reference to this type -
<https://www.byond.com/forum/post/2544111#comment25180960>
After some cursory poking, /dm_filter is a hidden type that can be used to manipulate filter instances without using the runtime search operator (:). There is no official reference material for it. It does not descend from datum, cannot be subtyped, and can only be created *successfully* by a valid call to proc/filter(...). All filter types create the same kind of /dm_filter but with different properties, of which I believe type is the only const.
A contributor on Bay tried to use it but checker doesn't know about it. I'm not familiar with the guts of suite, but I think this is all that's necessary?
This is so IconDiffBot2 can operate purely in memory.
Metadata::from_bytes and IconFile::from_bytes take an &[u8] as the
sequence of bytes of a valid PNG+DMI file to produce their types.
Image::to_bytes gives a Vec<u8> which is just the output from
png::Encoder.
There is no Image::from_bytes because there's no reason not to use
IconFile::from_bytes, and there is no to_bytes for Metadata or IconFile
because they don't support to_file either. IconFile could, but it's
unnecessary.
Co-authored-by: Tad Hardesty <tad@platymuus.com>
notable changes:
- dmdoc: changes in brokenlink callback, no longer takes &str directly, borrows instead, so the closure must not outlive the doc its parsing
- langserver: some structs's member names are changed to uppercase, doc-id version is not an option<> anymore, some u64s are changed to be u32 instead, oneof enum uses in some cases
Co-authored-by: Tad Hardesty <tad@platymuus.com>
This resolves every single error raised by [Clippy](https://github.com/rust-lang/rust-clippy) for every active crate. Most are formatting or redundant code, there's only a few things that are functionally better.
* Implement PartialEq manually on Prefab and Pop to ensure consistency with Hash
* Disable clippy::if_same_then_else for simple elif chains
* Add a standard path for the unit test codebase so I don't have to worry about git add
* Collapse nested if within smart cables loop
* mem::replace(x, X::default()) -> mem::take(x)
* Single character strings -> chars
* Use a const fn for Constant::null, unwrap_or -> unwrap_or_else calls to it
* writeln! always uses \n alone
* Address complex type signatures with type aliases
* PickArgs shouldn't have Box in it's type alias
* Random unnecessary variable access
* Add and remove closures where appropriate
* No more redundant field names in struct initialization
* Remove unnecessary references
* Use strip_prefix instead of starts_with -> [..]
* Use Range.contains instead of >= && <= comparisons
* Group hex literals by 4
* Elide lifetimes wherever possible
* Replace match with matches! wherever possible
* Remove needless borrows
* Control flow cleanup
* Misc signature & redundancy fixes
* Rename functions or move them to the correct std Trait
* Just ignore the too many arguments lint on the big ugly functions
* require! wasn't doing anything in tree_block.
tree_entries returns a Status<()>, thus passing () to self.require.
Macro expansion:
```rust
fn tree_block(
&mut self,
current: NodeIndex,
proc_kind: Option<ProcDeclKind>,
var_type: Option<VarTypeBuilder>,
) -> Status<()> {
match self.exact(Token::Punct(Punctuation::LBrace))? {
Some(x) => x,
None => return Ok(None),
};
Ok(Some({
let v = self.tree_entries(
current,
proc_kind,
var_type,
Token::Punct(Punctuation::RBrace),
);
self.require(v)?
}))
}
fn tree_entries(
&mut self,
current: NodeIndex,
proc_kind: Option<ProcDeclKind>,
var_type: Option<VarTypeBuilder>,
terminator: Token,
) -> Status<()> {
```
* Fix large enum variants
Co-authored-by: Tad Hardesty <tad@platymuus.com>
* Clarify intent on some of the linted code
* bugfix: fix part of the proc tree not being traversed for sleeps/purity when overrides are present
* add unit test
* copypasting whitespace be like
Co-authored-by: Fira <mekkiti@gmail.com>
A pattern such as
```dm
switch(rand(1, 3))
if(1)
foo()
if(2)
bar()
if(3)
baz()
```
is not uncommon in SS13 code. Sadly, it is also not uncommon for people to add new cases to a longer switch of this form and forget to change the `rand` arguments at the top. This PR adds a linter warning for when the `rand` range is not fully covered by the cases and for when a case lies completely outside of the `rand` range.
This currently generates 7 warnings on tgstation master. Sadly, it is (at least to me) not quite clear if the omissions of a part of the `rand` range are intentional there or not. However, I believe that even if they are intentional it is worth adding the missing part of the range as a case with empty body explicitly to make the intent clear to future developers.
As a less related (but perhaps more important) addition this PR now also adds a warning for switch branches of the form `if(X || Y)` which is pretty much always a mistake and should instead be `if(X, Y)`. This lint generates 5 warnings on current tg code, seemingly all being actual mistakes.
Fixes a case where parse errors could refer to LocatedTokens with
locations starting at 0,0 instead of at where the input string was
actually taken from.
