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

[drape] Fixed MutableLabelHandle mutation buffer's size. #8131

Merged
merged 1 commit into from
May 10, 2024
Merged

Conversation

vng
Copy link
Member

@vng vng commented May 10, 2024

Should fix #8128 and linked issues.
Fixes #8127
Fixes #8108

@vng vng requested a review from biodranik May 10, 2024 00:37
@vng vng added the Drape Drape OpenGL, Vulkan and Metal graphics rendering engine label May 10, 2024
@rtsisyk
Copy link
Contributor

rtsisyk commented May 10, 2024

Please mention the original change that has caused the regression.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

The fix doesn't work, the label is not cleared properly
telegram-cloud-photo-size-2-5327980903365990279-y

@biodranik
Copy link
Member

Please mention the original change that has caused the regression.

The original change was done 10 years ago.

@biodranik
Copy link
Member

@vng looks like all max 50 chars/vertexes should always be rendered for mutable labels, including the original string and remaining empty characters. Maybe add a suffix from a space char for up to max 50 chars limit?

@vng vng force-pushed the vng-fix branch 2 times, most recently from ba0c801 to c180913 Compare May 10, 2024 12:53
@rtsisyk
Copy link
Contributor

rtsisyk commented May 10, 2024

Please mention the original change that has caused the regression.

The original change was done 10 years ago.

Why, then, did this issue emerge in this release?

@biodranik
Copy link
Member

Why, then, did this issue emerge in this release?

It has been reproduced in many other releases since the beginning of OM. Now we have got a detailed stack trace and a better understanding of what's going on.

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

I wonder how it was cleared before? :)

auto const dataPointer = static_cast<MutableLabel::DynamicVertex *>(mutator->AllocateMutationBuffer(byteCount));
std::copy(result.m_buffer.begin(), result.m_buffer.end(), dataPointer);

dp::BindingInfo const & binding = MutableLabel::DynamicVertex::GetBindingInfo();
dp::OverlayHandle::TOffsetNode offsetNode = GetOffsetNode(binding.GetID());
dp::OverlayHandle::TOffsetNode const node = GetOffsetNode(binding.GetID());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dp::OverlayHandle::TOffsetNode const node = GetOffsetNode(binding.GetID());
dp::OverlayHandle::TOffsetNode const & node = GetOffsetNode(binding.GetID());

text.append("...");
}

strings::UniString uniText = bidi::log2vis(text);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
strings::UniString uniText = bidi::log2vis(text);
strings::UniString const uniText = bidi::log2vis(text);

@vng
Copy link
Member Author

vng commented May 10, 2024

I wonder how it was cleared before? :)

IDK, the mutator always read m_maxLength from the buffer, luckily to have some stuff in this buffer, that was interpreted as empty vertices ..

Signed-off-by: Viktor Govako <viktor.govako@gmail.com>
@biodranik biodranik merged commit 2f0eb98 into master May 10, 2024
15 checks passed
@biodranik biodranik deleted the vng-fix branch May 10, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Drape Drape OpenGL, Vulkan and Metal graphics rendering engine
Projects
None yet
3 participants