Skip to content

Add drawFill property to custom types to allow disabling the color fill #3564

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

Merged
merged 10 commits into from
Feb 17, 2023

Conversation

dogboydog
Copy link
Contributor

@dogboydog dogboydog commented Feb 4, 2023

  • Make drawFill value for types load properly when a project is opened
  • Add a toggle to the types editor that allows you to set the property
    • Decide on placement of the toggle - Next to the color button for a type, hopefully this is OK
  • Export the drawFill value to the custom types JSON
  • Hook up the value of drawFill to OrthogonalRenderer::drawMapObject and IsometricRenderer::drawMapObject to conditionally disable the fill.
  • Implement PR feedback
    Respect this setting for Point objects as well?
    Fix Configurable setting to make objects display no fill color #3312

Feel free to squash if merged

@dogboydog
Copy link
Contributor Author

From @bjorn :
The easiest for now would be to keep the rendering there, but to pass in the fillColor from MapObjectItem::paint

Sorry but I couldn't quite understand how MapObjectItem::paint connects to the drawMapObject methods. I see the line where you set the opacity to 50. And I see your example of reading the type values I believe here:

    // See if this object's class has a color associated with it
    if (auto type = Object::propertyTypes().findClassFor(effectiveClass, *this))
        return type->color;

But I don't quite get how to set the fill color in MapObjectItem::paint so that it would be used in drawMapObject

JSON export worked and import seemed to as well, but when I reopen Tiled it doesn't seem to be automatically getting this value correctly so I probably missed somewhere where it gets initialized.

image

Updated editor with toggle - might be awkward to have this on the "Use as" line so let me know if you want me to move it somewhere else
image

@bjorn bjorn force-pushed the 3312-class-fill-property branch from 5c4fec4 to 873f820 Compare February 4, 2023 20:50
@dogboydog
Copy link
Contributor Author

dogboydog commented Feb 4, 2023

I'm thinking maybe to the right of the color at the top right would be a better place for the checkbox
image

@dogboydog
Copy link
Contributor Author

dogboydog commented Feb 11, 2023

This is partially working now, let me know if it's not what you had in mind.
One thing I need to find where to implement is saving and restoring the drawFill property from the project file. When I reopen Tiled after setting drawFill to false it default back to true with my current version

Test map/project
drawFill.zip

@dogboydog
Copy link
Contributor Author

eishiya raised the question of if this might be better served as an opacity slider rather than a boolean checkbox, for some more flexibility. What do you think?

@dogboydog
Copy link
Contributor Author

dogboydog commented Feb 11, 2023

I was actually mostly there on restoring the property from the project file. I just was missing an assignment.

Example screenshot of the feature on an orthogonal map
image

Seems there's a bug though -- I might have done something wrong to cause this - when I first open the test isometric map I made, there are objects drawn in the wrong place. Mousing a tool over them clears them.

image

Also eishiya and I noticed that this isn't working for "point" type objects. I guess those go through a different code path. Not a huge deal for me but I can change it just not sure what I missed.

An example project
drawFill_multiple_maps.zip

@dogboydog dogboydog marked this pull request as ready for review February 11, 2023 22:01
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but I think it could be improved a little. See the comments. :-)

@bjorn
Copy link
Member

bjorn commented Feb 13, 2023

  • Respect this setting for Point objects as well?

While I think point objects do need some customization, I don't think they need to respect this fill setting.

@dogboydog
Copy link
Contributor Author

dogboydog commented Feb 13, 2023 via email

@bjorn bjorn added this to the Tiled 1.10 milestone Feb 15, 2023
dogboydog and others added 3 commits February 16, 2023 00:28
* Pass MapObjectColors by 'const &'
* Implement MapObject::effectiveColor based on MapObject::effectiveColors
* Fixed MapObject::effectiveColors to fall back properly
* MapObjectItem now stores a MapObjectColors instance and uses it when
  painting
* Simplified MapObjectItem::paint for TMX Viewer
@bjorn bjorn merged commit f96fb82 into mapeditor:master Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable setting to make objects display no fill color
2 participants