search

craftworkgames / MonoGame.Extended

Extensions to make MonoGame more awesome
http://www.monogameextended.net/
Other
1.42k stars 322 forks source link

[Tiled] Build options for tile source images (latest build) #526

Open Raflos10 opened 6 years ago

Raflos10 commented 6 years ago

Since switching to the latest build of extended, I noticed that the importer for tiled map files (.tmx) automatically processes tile source images even if they aren't referenced in the .mgcb file. And even if they aren't "embedded" in the map file. The problem is that even if you add the tile images into the mgcb file, it still chooses to load the ones in the .tmx file. So currently it seems that there is no way to change build options (such as "generate mipmaps") for tile images.

craftworkgames commented 6 years ago

Okay, thanks for letting us know. I believe this change was made a in a recent PR. I haven't really had a chance to properly look into it but I'll keep it in mind as I work on the Tiled stuff.

Out of curiosity, do you have a real world use case for generating mipmaps for tiled images or was that just an example off the top of your head?

Raflos10 commented 6 years ago

Yeah I use mipmaps for keeping consistent image quality when playing at a lower game resolution. It's also useful for if your game camera can zoom out. Otherwise the images really flicker a lot, especially when panning the camera slowly.

stefanrbk commented 6 years ago

That is a good point. This should not very difficult to add. When importing image files, it does bypass the .mgcb file and uses the default settings. The importer currently feeds the transparency color into the processor, so we can pass other options in with no problem. See here in PR #521 The only issue I can see is how we want to pass those options to the processor. We could pass it in as Properties for the tileset/image layer and filter them out in the processor.

Raflos10 commented 6 years ago

Thanks for pointing out the location in the code. I just made a pull request with your suggestion of using tiled properties to specify the build parameters. Hopefully it works, I haven't tested it yet. Ultimately it would probably be better to have them specified in the content pipeline tool somehow.

Raflos10 commented 6 years ago

Tested with the mipmaps property, seems to work.

craftworkgames commented 6 years ago

We could pass it in as Properties for the tileset/image layer and filter them out in the processor.

Whoa, whoa. Hold on a second. Yes, we could do that but it doesn't really seem like the right solution to this problem. Right?

I just made a pull request with your suggestion of using tiled properties to specify the build parameters. Hopefully it works, I haven't tested it yet.

I appreciate the enthusiasm and I'm more than happy to keep the PR open for further discussion but I don't think this is really the change we should be making. Especially if it's not tested.

Ultimately it would probably be better to have them specified in the content pipeline tool somehow.

Yes, I think we need to figure out how to make this work in the Pipeline tool.

stefanrbk commented 6 years ago

Ultimately it would probably be better to have them specified in the content pipeline tool somehow.

Yes, I think we need to figure out how to make this work in the Pipeline tool.

Unfortunately using BuildAsset to link assets together while building the project does bypass the Pipeline tool, so the processor options need to be passed from the asset which is already being processed. I'm not opposed to passing them as Properties on the map as it is uncommon to have to change these or use many of them.

stefanrbk commented 6 years ago

Only thing I would change is to probably remove the properties from the image once a match for a processor property is found. I don't think we need to pass around the property after the asset is loaded since it is only there to load the image correctly.

Raflos10 commented 6 years ago

I don't think this is really the change we should be making. Especially if it's not tested.

I agree it probably shouldn't be the permanent solution to this problem, I just put it together as a hack solution so that it's at least possible to have access to those build options for now. It's fine if you don't merge the changes, at least the code is out there in case someone else needs it. If I run into any other problems I'll let you guys know. Great project by the way!

craftworkgames commented 6 years ago

@stefanrbk I was doing some work on the Tiled Pipeline code tonight and my gut feeling at the moment is that we should be able to solve this problem.

Unfortunately I was having trouble finding any decent documentation on how the BuildAsset stuff works. Microsoft seems to have taken down the old XNA references. Do you know of any good sources of info for this stuff?

stefanrbk commented 6 years ago

@craftworkgames I looked at MonoGame's documentation for ContentProcessorContext

stefanrbk commented 6 years ago

@Raflos10

I just put it together as a hack solution

This is probably how I would have made this change myself!

Raflos10 commented 6 years ago

@stefanrbk Oh please, you flatter me pokéfriend.

stefanrbk commented 6 years ago

Well, I probably would have used a case, but same thing! 😉

Raflos10 commented 6 years ago

Right... For some reason I didn't think you could use switch statements with strings. Good to know!

craftworkgames commented 6 years ago

Right... For some reason I didn't think you could use switch statements with strings. Good to know!

It's part of a relatively new feature in C# called pattern matching. It can do all sorts of cool things 😄

Raflos10 commented 6 years ago

Wow, it just happens that I was looking for a way to use switch statements with types so this helps me a lot. Thanks!