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

not actually a PR #289

Closed
wants to merge 30 commits into from
Closed

not actually a PR #289

wants to merge 30 commits into from

Conversation

@s3bk
Copy link
Contributor

s3bk commented Mar 26, 2020

I am making this to discuss changes I made and whether to merge some of them.

s3bk added 29 commits Jan 31, 2020
Put the DebugUiPresenter behind the debug_ui feature flag
Clean up unused imports
let travis build pathfinder_webgl
Copy link
Contributor Author

s3bk left a comment

I marked all changes that I think could be worth merging.

/// Add other path to this path.
///
/// note: any non-closed subpaths will be closed
pub fn add_path(&mut self, mut other: Path2D) {
self.flush_current_contour();
other.flush_current_contour();

// FIXME(s3bk) should consume segments instead of cloning
for contour in other.outline.contours() {
self.outline.push_contour(contour.clone());
}
}

/// Transform the path with the given transformation.
/// returns the transformed path.
///
/// note: any non-closed subpaths will be closed
pub fn transform(mut self, transform: &Transform2F) -> Path2D {
self.flush_current_contour();
self.outline.transform(transform);
self
}
Comment on lines 572 to 593

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

These two can probably be added without problems. They are used by vector.

#[inline]
pub fn clear(&mut self) {
self.contours.clear();
self.bounds = RectF::default();
}

#[inline]
pub fn len(&self) -> usize {
self.contours.len()
}

Comment on lines 123 to 133

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

also used by vector

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

these are used to improve performance (and they do a good job at it).
Turns out copying the known length into a newly allocated buffer, and then reusing the old one for the next path, is a lot faster than starting with an empty buffer each time and transferring ownership.

#[inline]
pub fn with_capacity(length: usize) -> Contour {
Contour {
points: Vec::with_capacity(length),
flags: Vec::with_capacity(length),
bounds: RectF::default(),
closed: false,
}
}
Comment on lines 241 to 249

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

used by vector

#[inline]
pub fn clear(&mut self) {
self.points.clear();
self.flags.clear();
self.bounds = RectF::default();
self.closed = false;
}

Comment on lines +332 to +339

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

used by vector

This comment has been minimized.

@s3bk

s3bk Mar 28, 2020

Author Contributor

this one?

pub extern crate log;

#[macro_export]
macro_rules! pa_log {
($($t:tt)*) => (
$crate::log::log!($($t)*)
)
}

#[macro_export]
#[cfg(feature="log_debug")]
macro_rules! pa_debug {
($($t:tt)*) => (
$crate::log::debug!($($t)*)
)
}
#[macro_export]
#[cfg(not(feature="log_debug"))]
macro_rules! pa_debug {
($($t:tt)*) => ()
}

#[macro_export]
#[cfg(feature="log_trace")]
macro_rules! pa_trace {
($($t:tt)*) => (
$crate::log::trace!($($t)*)
)
}
#[macro_export]
#[cfg(not(feature="log_trace"))]
macro_rules! pa_trace {
($($t:tt)*) => ()
}

#[macro_export]
#[cfg(feature="log_info")]
macro_rules! pa_info {
($($t:tt)*) => (
$crate::log::info!($($t)*)
)
}
#[macro_export]
#[cfg(not(feature="log_info"))]
macro_rules! pa_info {
($($t:tt)*) => ()
}

#[macro_export]
#[cfg(feature="log_warn")]
macro_rules! pa_warn {
($($t:tt)*) => (
$crate::log::warn!($($t)*)
)
}
#[macro_export]
#[cfg(not(feature="log_warn"))]
macro_rules! pa_warn {
($($t:tt)*) => ()
}

#[macro_export]
macro_rules! pa_error {
($($t:tt)*) => (
$crate::log::error!($($t)*)
)
}
Comment on lines +1 to +67

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

I am not sure about this. But I also could not work without it.

This comment has been minimized.

@pcwalton

pcwalton Mar 27, 2020

Collaborator

Just because of compilation time?

This comment has been minimized.

@s3bk

s3bk Mar 27, 2020

Author Contributor

It can make things more difficult if you forget to enable the appropriate feature.
(Then again, most users would probably not have to debug pathfinder anyway.)
I ran into this myself…

This comment has been minimized.

@s3bk

s3bk Mar 27, 2020

Author Contributor

I just don't have any better solution…

This comment has been minimized.

@pcwalton

pcwalton Mar 27, 2020

Collaborator

I'm not sure what you mean by forgetting to enable the appropriate feature, could you elaborate?

impl<'a, L: RenderCommandListener> SceneBuilder<'a, L> {
pub(crate) fn new(
scene: &'a Scene,
built_options: &'a PreparedBuildOptions,
listener: Box<dyn RenderCommandListener>,
) -> SceneBuilder<'a> {
listener: L,
) -> SceneBuilder<'a, L> {
let effective_view_box = scene.effective_view_box(built_options);
Comment on lines +87 to +93

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

I replaced the trait object with a generic, as in WebAssembly it is all single threaded and are no runtime decisions possible. I did not measure the performance gain.

This comment has been minimized.

@pcwalton

pcwalton Mar 27, 2020

Collaborator

I used a trait object to improve compilation time and because the overhead of vtable dispatch should be minimal.

#[inline]
pub fn packed_eq(self, other: I32x2) -> U32x2 {
U32x2([
if self[0] == other[0] { !0 } else { 0 },
if self[1] == other[1] { !0 } else { 0 },
])
}

#[inline]
pub fn packed_gt(self, other: I32x2) -> U32x2 {
U32x2([
if self[0] > other[0] { !0 } else { 0 },
if self[1] > other[1] { !0 } else { 0 },
])
}

#[inline]
pub fn packed_le(self, other: I32x2) -> U32x2 {
U32x2([
if self[0] <= other[0] { !0 } else { 0 },
if self[1] <= other[1] { !0 } else { 0 },
])
}

#[inline]
pub fn packed_lt(self, other: I32x2) -> U32x2 {
U32x2([
if self[0] < other[0] { !0 } else { 0 },
if self[1] < other[1] { !0 } else { 0 },
])
}
Comment on lines 502 to 532

This comment has been minimized.

@s3bk

s3bk Mar 26, 2020

Author Contributor

These should probably be added.

pcwalton added a commit that referenced this pull request Mar 26, 2020
pcwalton added a commit that referenced this pull request Mar 27, 2020
@s3bk s3bk closed this Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.