Added alphaonly imageloader parameter. #178
Conversation
pytmx/util_pygame.py
Outdated
| filename: The filename, including path, to be loaded. | ||
| colorkey: The colorkey for the image. | ||
|
|
||
| Optional named parameters: |
There was a problem hiding this comment.
curious why the optional parameters need to be in their own section? there are optional parameters in another part of the code in pr which were not broken out into their own section. also minor point of preference, periods are used when terminating sentences and not sentence fragments. for example, A function to load tile images. is not a complete sentence and shouldn't be terminated with any punctuation. i don't care enough to make you revert it, but i probably will at some later point.
There was a problem hiding this comment.
I broke them out for only one reason. They had previously been omitted from the comment docs and I assume there may have been a thought process behind that and I wanted to respect it.
Regarding this and the periods and sentences, etc., I struggled with all the changes to the comment docs. I wanted to update the comment docs to match my changes, but I didn't want to change any more than necessary. I tried to update only the parts that I had touched. In doing so, most of my intent was to do so with as much consistency as possible. In one or two cases, I may have thought I was making a correction as well. I am truly not offended if you want to dictate any kind of changes to the comment docs for this or any other reason.
I didn't see another place where the optional parameters were not broken out to a separate section. I only saw a place where they were positional / non-optional parameters, which would be consistent with what I did here (I thought :) )
There was a problem hiding this comment.
there is no reason to change existing comments unless the code under them changes, context changes, etc. any random aesthetic changes just creates churn and needless discussions like this. please remove the Optional named parameters: heading as its not needed and confusing. if you "struggled" with adding comments, perhaps the best solution would have been to leave the existing ones alone, and use the existing style for your own. or not. but not adding unneeded changes would be great.
There was a problem hiding this comment.
I've removed the literary corrections and reverted to the previous style. The new comment docs are now consistent with that.
pytmx/pytmx.py
Outdated
| loader = self.image_loader(path, colorkey, tileset=ts) | ||
| kwargs_to_pass = self.image_loader_kwargs.copy() | ||
| kwargs_to_pass["tileset"] = "ts" | ||
| loader = self.image_loader(path, colorkey, **kwargs_to_pass) |
There was a problem hiding this comment.
what is the point of kwargs_to_pass?
There was a problem hiding this comment.
As you see in line 704 and 716, I would be happy to pass the kwargs directly through. Without passing the kwargs, the new alphaonly and the already-existing pixelalpha parameters do not get passed all the way from dunder-init__() to the image loader and the values get lost.
The reason for the actual variable kwargs_to_pass here is to pass along the kwargs that were introduced in the dunder-init__() but also inject the "ts" value into the dictionary to maintain its existing functionality. I thought I could do it without using a variable but after much tinkering, I couldn't figure out a way. Maybe my Python is weak. :/ If there is a clean way to omit the variable itself (if that's what you're referring to), that was my original hope anyway. If it's just an explanation you're after, I hope this makes it more clear.
There was a problem hiding this comment.
yeah, im still not understanding why there is a need for some additional dictionary. could you illustrate the purpose in code? or just drop it?
There was a problem hiding this comment.
I've removed the additional dictionary.
|
Thank you for reviewing this. I'm happy to make any changes you suggest to get this merged. I'd really like to have these changes available in PIP. |
|
tbh, im personally hesitant to add more parameters to the loader function, but i will entertain the idea. because to me, if there is a bug where some images have transparent tiles, but that is not determined properly, then the bug should be fixed, rather than adding some workaround for the problem. workarounds for bugs become "features" and a maintenance burden. actually fixing the transparent pixel bug would be much more appropriate than just covering it up and adding more "features". |
The new alphaonly parameter circumvents the smart part of the smart_convert() method when pygame misbehaves with non-transparent tiles. This should be a non-breaking change.
Here is the problem I have: There are certain tiles which I created without transparency will be determined by pygame's .mask() method to have no transparency. The smart_convert() method then correctly calls .convert() on them. But then pygame draws these tiles without drawing the black pixels. As far as I can tell, the problem is in PyGame. It's possible that it's something weird about my tilesheet imaages, but it only does it with certain tiles. This is not a bug in pytmx. I've had this problem for YEARS with pygame. Whether it's me or them, the solution to my problem is to use convert_alpha(). This new parameter gives me the control to force pytmx to do that. Without this change, I had to create a custom image loader to solve this problem.