Skip to content
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

Per-object opacity #4031

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Per-object opacity #4031

wants to merge 10 commits into from

Conversation

jcbk101
Copy link

@jcbk101 jcbk101 commented Aug 15, 2024

Add opacity to tile objects with images. Shapes are not supported.

jcbk101 and others added 8 commits August 14, 2024 21:30
initial push
Commit history can be used to see who changed what. :-)
I didn't see a good reason to limit this feature to tile objects.
It's never set to anything else than 1.0.
The selection outline should stay clearly visible.
Opacity is irrelevant here since it doesn't change when an object is
moved.
We don't need these values to stay the same, so we can put Opacity
after Rotation like everywhere else in the code.
@bjorn
Copy link
Member

bjorn commented Aug 15, 2024

@jcbk101 I'm sorry for all the small commits and the notifications they might have triggered. In any case, I've cleaned things up a bit:

  • Removed "BONGO" from the code
  • Removed some unused variables (Cell::_opacity and MovingObject::oldOpacity)
  • Enabled editing opacity for all map objects, not just tile objects

Was there any particular reason you had the Properties view check for tile objects? I seems like it was due to a possible previous implementation based on Cell::_opacity that would have only worked for tile objects?

In any case I'd say we give the change a bit of time for testing, but it mostly just still needs an entry added to the NEWS.md.

Thank you for implementing this!

@bjorn
Copy link
Member

bjorn commented Aug 15, 2024

There are some places that should be affected by this but currently are not:

  • The Mini-map view, Export as Image and various other places where maps are rendered. The object's opacity will need to be taken into account by MiniMapRenderer::renderToImage for this.

  • The tmxrasterizer tool. This one has its own map rendering code due to supporting a number of additional options, and will need similar adjustments in TmxRasterizer::drawMapLayers.

Also, the opacity still needs to be added to the map format documentation in the manual (for TMX and JSON).

@jcbk101
Copy link
Author

jcbk101 commented Aug 15, 2024

There are some places that should be affected by this but currently are not:

  • The Mini-map view, Export as Image and various other places where maps are rendered. The object's opacity will need to be taken into account by MiniMapRenderer::renderToImage for this.
  • The tmxrasterizer tool. This one has its own map rendering code due to supporting a number of additional options, and will need similar adjustments in TmxRasterizer::drawMapLayers.

Also, the opacity still needs to be added to the map format documentation in the manual (for TMX and JSON).

I did see this code, but I was not sure I wanted to support it as I originally was supposedly supporting to feature for personal use.
😞

As for documentation, should I handle this? What about Tiled API web docs?

@jcbk101
Copy link
Author

jcbk101 commented Aug 15, 2024

@jcbk101 I'm sorry for all the small commits and the notifications they might have triggered. In any case, I've cleaned things up a bit:

  • Removed "BONGO" from the code
  • Removed some unused variables (Cell::_opacity and MovingObject::oldOpacity)
  • Enabled editing opacity for all map objects, not just tile objects

In any case I'd say we give the change a bit of time for testing, but it mostly just still needs an entry added to the NEWS.md.

Thank you for implementing this!

What you are seeing is me getting familiar with the source code. I did not spend much time reverse engineering the project as I was goal oriented. This is why objectselectiontool has opacity effects with said changes. This was not the intended action of course. But seeing the potential to have an actual contribution, doing it correctly is important.

Was there any particular reason you had the Properties view check for tile objects? I seems like it was due to a possible previous implementation based on Cell::_opacity that would have only worked for tile objects?

When first implementing the changes, I was attempting to access mapObject's opacity variable inside of the render / drawing function.

@bjorn
Copy link
Member

bjorn commented Aug 15, 2024

I did see this code, but I was not sure I wanted to support it as I originally was supposedly supporting to feature for personal use. 😞

That's alright! If you'd like you can help with finishing the remaining parts now, or wait for me to get around to it. :-)

As for documentation, should I handle this? What about Tiled API web docs?

Yes, feel free to have a look into adjusting the manual! The relevant pages are at:

  • docs/reference/tmx-map-format.rst
  • docs/reference/tmx-changelog.rst
  • docs/reference/json-map-format.rst
  • And indeed the scripting API docs need to be adjusted in docs/scripting-doc/index.d.ts

@jcbk101
Copy link
Author

jcbk101 commented Aug 15, 2024

I did see this code, but I was not sure I wanted to support it as I originally was supposedly supporting to feature for personal use. 😞

That's alright! If you'd like you can help with finishing the remaining parts now, or wait for me to get around to it. :-)

As for documentation, should I handle this? What about Tiled API web docs?

Yes, feel free to have a look into adjusting the manual! The relevant pages are at:

  • docs/reference/tmx-map-format.rst
  • docs/reference/tmx-changelog.rst
  • docs/reference/json-map-format.rst
  • And indeed the scripting API docs need to be adjusted in docs/scripting-doc/index.d.ts

Thanks for the info. I will make adjustments to my fork since a pull is not ready. Then after work, I will work on documentation.

Remove build files
@jcbk101
Copy link
Author

jcbk101 commented Aug 15, 2024

Okay, minimaprenderer and tmxrasterizer seem to be working.

I added simple updates (copy/paste) to this docs and was sure to use object as the reference.

  • docs/reference/tmx-map-format.rst
  • docs/reference/json-map-format.rst

I did not bother with tmx-changelog and API docs was also left alone.

Opacity adjusted for tmxrasterizer and minimaprederer
@@ -203,6 +203,8 @@ void MiniMapRenderer::renderToImage(QImage &image, RenderFlags renderFlags) cons
painter.translate(-origin);
}

painter.setOpacity(object->opacity());
Copy link
Member

@bjorn bjorn Aug 16, 2024

Choose a reason for hiding this comment

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

We'll need to store the above layer->effectiveOpacity() in a variable and set the opacity to layerOpacity * object->opacity() here.

Copy link
Author

Choose a reason for hiding this comment

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

Wow. I don't think I would have even considered that. This is one of my fears of being a contributor. I do not know the full requirements for such a project. I am just learning qt to be honest.

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.

2 participants